Re: [RFC] Storing same-filled pages without a zswap_entry

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2024/3/21 05:31, Yosry Ahmed wrote:
> On Wed, Mar 20, 2024 at 2:19 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>>
>> On Wed, Mar 20, 2024 at 05:07:21PM -0400, Johannes Weiner wrote:
>>> On Wed, Mar 20, 2024 at 01:49:17PM -0700, Yosry Ahmed wrote:
>>>> Hey folks,
>>>>
>>>> I was looking at cleaning up the same-filled handling code in zswap,
>>>> when it hit me that after the xarray conversion, the only member of
>>>> struct zwap_entry that is relevant to same-filled pages is now the
>>>> objcg pointer.
>>>>
>>>> The xarray allows a pointer to be tagged by up to two tags (1 and 3),
>>>> so we can completely avoid allocating a zswap_entry for same-filled
>>>> pages by storing a tagged objcg pointer directly in the xarray
>>>> instead.
>>>>
>>>> Basically the xarray would then either have a pointer to struct
>>>> zswap_entry or struct obj_cgroup, where the latter is tagged as
>>>> SAME_FILLED_ONE or SAME_FILLED_ZERO.
>>>>
>>>> There are two benefits of this:
>>>> - Saving some memory (precisely 64 bytes per same-filled entry).
>>>> - Further separating handling of same-filled pages from compressed
>>>> pages, which results in some nice cleanups (especially in
>>>> zswap_store()). It also makes further improvements easier (e.g.
>>>> skipping limit checking for same-filled entries).

I also think this is a good idea. :) Which could simplify the code too.

>>>
>>> This sounds interesting.
>>>
>>> Where would you store the byte value it's filled with? Or would you
>>> limit it to zero-filled only?
>>
>> The dumb thing about objcg is that for same-filled entries we really
>> only need it for bumping ZSWPIN. Nothing else. entry->length is 0 for
>> them, so even though we call the charge function, it doesn't actually
>> do anything.
>>
>> Loading them is cheap and doesn't involve decompression. An argument
>> could be made to exclude them from ZSWPOUT and ZSWPIN entirely.
>>
>> Or cheat a little and bump ZSWPIN for current->objcg instead -
>> probably good enough to make excessive thrashing discoverable by the
>> workload that's directly affected.
>>
>> Then you could get rid of the objcg pointer and use the xarray slot
>> for whatever else you'd want.
> 
> Yeah it's only useful for the stats. Using current->objcg would work,
> and should be ultimately pointing to the same memcg in *most* cases, I

In some cases where the current objcg is not "correct", the testcases in
test_zswap.c may break? Maybe we can use swap_cgroup info to charge the
stats to the correct memcg? Not sure if this is feasible.

> assume. We still wouldn't be able to store a full word as we do today,
> because the xarray needs 1 bit for its own usage. So the same-filled
> implementation would still need to change from repeated words (8
> bytes) to something smaller -- or we can just allocate a separate
> struct for same-filled pages.

Yes, this seems an unavoidable limit of value in xarray...

Thanks.