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

[Johannes Weiner <hannes@cmpxchg.org>]

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.