[RFC] Storing same-filled pages without a zswap_entry

[RFC] Storing same-filled pages without a zswap_entry

[Yosry Ahmed]

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).

The disadvantage is obviously the complexity needed to handle two
different types of pointers in the xarray, although I think with the
correct abstractions this is not a big deal.

I have some untested patches that implement this that I plan on
testing and sending out at some point, the reason I am sending this
RFC now is to gauge interest. I am not sure how common same-filled
pages are. Unfortunately, this data is not easy to collect from our
fleet (still working on it), so if someone has data from actual
workloads that would be helpful.

Running the kernel build test only shows a small amount of same-filled
pages landing in zswap, but I am thinking maybe actual workloads have
more zerod pages lying around.

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

[Johannes Weiner]

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).

This sounds interesting.

Where would you store the byte value it's filled with? Or would you
limit it to zero-filled only?

> The disadvantage is obviously the complexity needed to handle two
> different types of pointers in the xarray, although I think with the
> correct abstractions this is not a big deal.
> 
> I have some untested patches that implement this that I plan on
> testing and sending out at some point, the reason I am sending this
> RFC now is to gauge interest. I am not sure how common same-filled
> pages are. Unfortunately, this data is not easy to collect from our
> fleet (still working on it), so if someone has data from actual
> workloads that would be helpful.
> 
> Running the kernel build test only shows a small amount of same-filled
> pages landing in zswap, but I am thinking maybe actual workloads have
> more zerod pages lying around.

In our fleet, same-filled pages seem to average pretty consistently at
10% of total stored entries.

I'd assume they're mostly zero-filled instead of other patterns, but I
don't have any data on that.

The savings from the entry would be a few megabytes per host, probably
not an argument by itself. But the code simplifications and shaving a
few cycles of the fast paths do sound promising.

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

[Yosry Ahmed]

On Wed, Mar 20, 2024 at 2:07 PM Johannes Weiner <hannes@cmpxchg.org> 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).
>
> This sounds interesting.
>
> Where would you store the byte value it's filled with? Or would you
> limit it to zero-filled only?

Oh yeah I should have explicitly stated this, it would be limited to
pages filled with the same bit (all 0s or all 1s), assuming that most
same-filled entries would be zero-filled anyway.

>
> > The disadvantage is obviously the complexity needed to handle two
> > different types of pointers in the xarray, although I think with the
> > correct abstractions this is not a big deal.
> >
> > I have some untested patches that implement this that I plan on
> > testing and sending out at some point, the reason I am sending this
> > RFC now is to gauge interest. I am not sure how common same-filled
> > pages are. Unfortunately, this data is not easy to collect from our
> > fleet (still working on it), so if someone has data from actual
> > workloads that would be helpful.
> >
> > Running the kernel build test only shows a small amount of same-filled
> > pages landing in zswap, but I am thinking maybe actual workloads have
> > more zerod pages lying around.
>
> In our fleet, same-filled pages seem to average pretty consistently at
> 10% of total stored entries.
>
> I'd assume they're mostly zero-filled instead of other patterns, but I
> don't have any data on that.

I think this would be key to know if this is doable. If most
same-filled pages are zero pages, we can limit zswap's same-filled
feature for those and do the optimization. Otherwise we cannot do it,
as we actually store an entire word now as the same-filled "value".

Alternatively, we can have a separate struct that only has objcg and
value for same-filled pages. This means we still use 16 bytes (on
x86_64) per same-filled entry. I think we can still get the code
cleanups by separating handling for same-filled pages though?

>
> The savings from the entry would be a few megabytes per host, probably
> not an argument by itself. But the code simplifications and shaving a
> few cycles of the fast paths do sound promising.

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

[Johannes Weiner]

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).
> 
> 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.

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

[Yosry Ahmed]

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).
> >
> > 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
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.

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

[Chris Li]

On Wed, Mar 20, 2024 at 2:07 PM Johannes Weiner <hannes@cmpxchg.org> 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).
>
> This sounds interesting.
>
> Where would you store the byte value it's filled with? Or would you
> limit it to zero-filled only?
>
> > The disadvantage is obviously the complexity needed to handle two
> > different types of pointers in the xarray, although I think with the
> > correct abstractions this is not a big deal.
> >
> > I have some untested patches that implement this that I plan on
> > testing and sending out at some point, the reason I am sending this
> > RFC now is to gauge interest. I am not sure how common same-filled
> > pages are. Unfortunately, this data is not easy to collect from our
> > fleet (still working on it), so if someone has data from actual
> > workloads that would be helpful.
> >
> > Running the kernel build test only shows a small amount of same-filled
> > pages landing in zswap, but I am thinking maybe actual workloads have
> > more zerod pages lying around.
>
> In our fleet, same-filled pages seem to average pretty consistently at
> 10% of total stored entries.
>
> I'd assume they're mostly zero-filled instead of other patterns, but I
> don't have any data on that.

That matches with my experience. All of them are just zero fill pages
for the kdump that I have looked at. It is about a few thousand of
zero fill pages if I remember correctly.

Chris

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

[Chengming Zhou]

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.

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

[Chris Li]

On Wed, Mar 20, 2024 at 1:49 PM Yosry Ahmed <yosryahmed@google.com> 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).
>
> The disadvantage is obviously the complexity needed to handle two
> different types of pointers in the xarray, although I think with the
> correct abstractions this is not a big deal.

Another idea is we can make the zero fill zswap entry immutable and
share by different pages. That way it does not depend on the xarray
pointer tagging. It can share the same code path with existing zero
fill page handling. Just don't free the zero fill entry.

I want to reserve the zswap xarray tagging for another purpose.

Just some food for thought. Might have less complexity because it
doesn't need to deal with two different kinds of pointers.

Chris

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

[Yosry Ahmed]

On Thu, Mar 21, 2024 at 11:40:32AM +0800, Chengming Zhou wrote:
> 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.

For cgroup v1, swap_cgroup will be cleared from
mem_cgroup_swapin_uncharge_swap() before the zswap load.

I think the current objcg will remain correct as long as swapin happens
from the same memcg as swapout (or if swapin happens from the parent
memcg and the swapout memcg was offlined).

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

[Yosry Ahmed]

On Wed, Mar 20, 2024 at 09:26:43PM -0700, Chris Li wrote:
> On Wed, Mar 20, 2024 at 1:49 PM Yosry Ahmed <yosryahmed@google.com> 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).
> >
> > The disadvantage is obviously the complexity needed to handle two
> > different types of pointers in the xarray, although I think with the
> > correct abstractions this is not a big deal.
> 
> Another idea is we can make the zero fill zswap entry immutable and
> share by different pages. That way it does not depend on the xarray
> pointer tagging. It can share the same code path with existing zero
> fill page handling. Just don't free the zero fill entry.

The only relevant item in zswap_entry for same-filled pages is objcg.
That cannot be shared by different pages anyway. So basically the
immutable zswap entry will just be a magic value that encodes all
zero-filled pages.

If we are all fine with only supporting zero-filled pages, then I don't
see why we should not just store the objcg in the xarray and tag it as
such. In fact, we only need to use one of the two available tags in this
case.

> 
> I want to reserve the zswap xarray tagging for another purpose.

Did you have another purpose in mind? or just for future expansion?

As I mentioned above, if we only support zero-filled pages, we only need
one of the two available tags. Also, this is an in-kernel change so I
think we could always take it back if we really need the tags in the
future.

> 
> Just some food for thought. Might have less complexity because it
> doesn't need to deal with two different kinds of pointers.

I think the complexity is not that much with the right abstractions, but
this is best discussed with actual code imo.

> 
> Chris
> 

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

[Johannes Weiner]

On Thu, Mar 21, 2024 at 06:44:54PM +0000, Yosry Ahmed wrote:
> On Thu, Mar 21, 2024 at 11:40:32AM +0800, Chengming Zhou wrote:
> > 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.
> 
> For cgroup v1, swap_cgroup will be cleared from
> mem_cgroup_swapin_uncharge_swap() before the zswap load.
> 
> I think the current objcg will remain correct as long as swapin happens
> from the same memcg as swapout (or if swapin happens from the parent
> memcg and the swapout memcg was offlined).

Swap readahead will pull in physically adjacent entries that may
belong to somebody unrelated.

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

[Yosry Ahmed]

On Thu, Mar 21, 2024 at 12:29 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Thu, Mar 21, 2024 at 06:44:54PM +0000, Yosry Ahmed wrote:
> > On Thu, Mar 21, 2024 at 11:40:32AM +0800, Chengming Zhou wrote:
> > > 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.
> >
> > For cgroup v1, swap_cgroup will be cleared from
> > mem_cgroup_swapin_uncharge_swap() before the zswap load.
> >
> > I think the current objcg will remain correct as long as swapin happens
> > from the same memcg as swapout (or if swapin happens from the parent
> > memcg and the swapout memcg was offlined).
>
> Swap readahead will pull in physically adjacent entries that may
> belong to somebody unrelated.

Right. For those as well the current objcg would be correct if they
are readahead from the same memcg as the one they were swapped out
from, but I understand your point that readahead makes that more
likely to not be the case.

I am slightly nervous about using the current objcg tbh, even though
it only affects the stats. It's just less straightforward this way. I
think I prefer either:
(a) Only supporting zero-filled pages and storing the objcg directly
in the xarray.
(b) Having a separate two-word struct to store objcg and value for
same-filled pages.

In both cases, we would need one tag bit to identify same-filled pages
in the xarray. (a) is more tempting for me, but I am not sure everyone
else agrees with dropping support for non-zero same-filled pages.

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

[Chris Li]

On Thu, Mar 21, 2024 at 11:50 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Wed, Mar 20, 2024 at 09:26:43PM -0700, Chris Li wrote:
>
> >
> > Another idea is we can make the zero fill zswap entry immutable and
> > share by different pages. That way it does not depend on the xarray
> > pointer tagging. It can share the same code path with existing zero
> > fill page handling. Just don't free the zero fill entry.
>
> The only relevant item in zswap_entry for same-filled pages is objcg.
> That cannot be shared by different pages anyway. So basically the

Ah, good points about the objcg can't be shared.

Chris

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

[Chris Li]

On Thu, Mar 21, 2024 at 12:58 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Thu, Mar 21, 2024 at 12:29 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Thu, Mar 21, 2024 at 06:44:54PM +0000, Yosry Ahmed wrote:
> > > > 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.
> > >
> > > For cgroup v1, swap_cgroup will be cleared from
> > > mem_cgroup_swapin_uncharge_swap() before the zswap load.
> > >
> > > I think the current objcg will remain correct as long as swapin happens
> > > from the same memcg as swapout (or if swapin happens from the parent
> > > memcg and the swapout memcg was offlined).
> >
> > Swap readahead will pull in physically adjacent entries that may
> > belong to somebody unrelated.
>
> Right. For those as well the current objcg would be correct if they
> are readahead from the same memcg as the one they were swapped out
> from, but I understand your point that readahead makes that more
> likely to not be the case.
>
> I am slightly nervous about using the current objcg tbh, even though
> it only affects the stats. It's just less straightforward this way. I
> think I prefer either:
> (a) Only supporting zero-filled pages and storing the objcg directly
> in the xarray.

We can add the zero-filled page as a special case to speed it up. I
don't think we should remove non zero values of the same fill pages
from zswap though. We can never declare the non zero same filled page
is not going to happen. In that case, the current same fill is still
better than going through the zsmalloc.

Chris

> (b) Having a separate two-word struct to store objcg and value for
> same-filled pages.
>
> In both cases, we would need one tag bit to identify same-filled pages
> in the xarray. (a) is more tempting for me, but I am not sure everyone
> else agrees with dropping support for non-zero same-filled pages.

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

[Yosry Ahmed]

On Thu, Mar 21, 2024 at 5:29 PM Chris Li <chrisl@kernel.org> wrote:
>
> On Thu, Mar 21, 2024 at 12:58 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Thu, Mar 21, 2024 at 12:29 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Thu, Mar 21, 2024 at 06:44:54PM +0000, Yosry Ahmed wrote:
> > > > > 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.
> > > >
> > > > For cgroup v1, swap_cgroup will be cleared from
> > > > mem_cgroup_swapin_uncharge_swap() before the zswap load.
> > > >
> > > > I think the current objcg will remain correct as long as swapin happens
> > > > from the same memcg as swapout (or if swapin happens from the parent
> > > > memcg and the swapout memcg was offlined).
> > >
> > > Swap readahead will pull in physically adjacent entries that may
> > > belong to somebody unrelated.
> >
> > Right. For those as well the current objcg would be correct if they
> > are readahead from the same memcg as the one they were swapped out
> > from, but I understand your point that readahead makes that more
> > likely to not be the case.
> >
> > I am slightly nervous about using the current objcg tbh, even though
> > it only affects the stats. It's just less straightforward this way. I
> > think I prefer either:
> > (a) Only supporting zero-filled pages and storing the objcg directly
> > in the xarray.
>
> We can add the zero-filled page as a special case to speed it up. I
> don't think we should remove non zero values of the same fill pages
> from zswap though. We can never declare the non zero same filled page
> is not going to happen. In that case, the current same fill is still
> better than going through the zsmalloc.

If we do this, we still need to support same-filled entries in
zswap_entry, which means we still cannot separate the logic
completely. Also, it would be weird to have zero-filled pages and
same-filled pages handled differently.

How common do we think same-filled but not zero-filled pages are? Are
there known user space usages or patterns that lead to this? The only
thing I can think of is if user space initializes a large array to the
same non-zero value, and then it gets swapped out before it is
modified.

If it is not common, perhaps it's not worth optimizing it.