Message-ID: <20250422150033.1867401-1-joshua.hahnjy@gmail.com>
Date: Tue, 22 Apr 2025 08:00:31 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Yosry Ahmed <yosryahmed@...gle.com>,
	Nhat Pham <nphamcs@...il.com>,
	akpm@...ux-foundation.org,
	hannes@...xchg.org,
	cerasuolodomenico@...il.com,
	sjenning@...hat.com,
	ddstreet@...e.org,
	vitaly.wool@...sulko.com,
	hughd@...gle.com,
	corbet@....net,
	konrad.wilk@...cle.com,
	senozhatsky@...omium.org,
	rppt@...nel.org,
	linux-mm@...ck.org,
	kernel-team@...a.com,
	linux-kernel@...r.kernel.org,
	david@...t.cz
Subject: Re: [PATCH 0/2] minimize swapping on zswap store failure


> > > I thought before about having a special list_head that allows us to
> > > use the lower bits of the pointers as markers, similar to the xarray.
> > > The markers can be used to place different objects on the same list.
> > > We can have a list that is a mixture of struct page and struct
> > > zswap_entry. I never pursued this idea, and I am sure someone will
> > > scream at me for suggesting it. Maybe there is a less convoluted way
> > > to keep the LRU ordering intact without allocating memory on the
> > > reclaim path.

> > So I've implemented your idea, using the lower 2 bits of the list_head's prev
> > pointer (last bit indicates whether the list_head belongs to a page or a
> > zswap_entry, and the second to last bit was repurposed for the second chance
> > algorithm).
> 
> Thanks a lot for spending time looking into this, and sorry for the
> delayed resposne (I am technically on leave right now).

Hi Yosry,

Thank you for getting back to me! I hope you are enjoying your leave : -)

> > For a very high level overview what I did in the patch:
> > - When a page fails to compress, I remove the page mapping and tag both the
> >   xarray entry (tag == set lowest bit to 1) and the page's list_head prev ptr,
> >   then store the page directly into the zswap LRU.
> 
> What do you mean by 'remove the page mapping'? Do you mean
> __remove_mapping()?

Yes -- but I am calling remove_mapping() to unfreeze with a refcount of 1
(zswap is now the sole owner of the page). 

> This is already called by reclaim, so I assume vmscan code hands over
> ownership of the page to zswap and doesn't call __remove_mapping(), so
> you end up doing that in zswap instead.

Yes! I changed reclaim logic to be aware that zswap can do this, so there
is a new switch case that simply continues through the folio list when
zswap steals the incompressible page (but we don't want to drop the page).

> > - In zswap_load, we take the entry out of the xarray and check if it's tagged.
> >   - If it is tagged, then instead of decompressing, we just copy the page's
> >     contents to the newly allocated page. 
> > - (More details about how to teach vmscan / page_io / list iterators how to
> >   handle this, but we can gloss over those details for now)
> > 
> > I have a working version, but have been holding off because I have only been
> > seeing regressions. I wasn't really sure where they were coming from, but
> > after going through some perf traces with Nhat, found out that the regressions
> > come from the associated page faults that come from initially unmapping the
> > page, and then re-allocating it for every load. This causes (1) more memcg
> > flushing, and (2) extra allocations ==> more pressure ==> more reclaim, even
> > though we only temporarily keep the extra page.
> 
> Hmm how is this worse than the status quo though? IIUC currently
> incompressible pages will skip zswap and go to the backing swapfile.
> Surely reading them from disk is slower than copying them?
> 
> Unless of course, writeback is disabled, in which case these pages are
> not being reclaimed at all today. In this case, it makes sense that
> reclaiming them makes accessing them slower, even if we don't actually
> need to decompress them.

Yes, sorry for the ambiguity -- this was specifically for the writeback
disabled case. My focus currently is on reducing the amount of CPU cycles
spent stuck on trying to compress incompressible pages. 

> I have a few thoughts in mind:
> 
> - As Nhat said, if we can keep the pages in the swapcache, we can avoid
>   making a new allocation and copying the page. We'd need to move it
>   back from zswap LRUs to the reclaim LRUs though.

Yes, Nhat and Shakeel have both offered the same perspective. I'm currently
working on this approach with help from Nhat. 

> - One advantage of keeping incompressible pages in zswap is preserving
>   LRU ordering. IOW, if some compressible pages go to zswap first (old),
>   then some incompressible pages (new), then the old compressible pages
>   should go to disk via writeback first. Otherwise, accessing the hotter
>   incompressible pages will be slower than accessing the colder
>   compressible pages. This happens today because incompressible pages go
>   straight to disk.
> 
>   The above will only materialize for a workload that has writeback
>   enabled and a mixture of both incompressible and compressible
>   workingset.

This makes sense to me. I'll take this into consideration when writing
benchmarks for this patch!

>   The other advantage, as you mention below, is preventing repeatedly
>   sending incompressible pages to zswap when writeback is disabled, but
>   that could be offset by the extra cost of allocations/copying.

Yes -- hopefully, keeping it in swapcache allows us to reap the benefits of
both worlds, minus the duplicated allocation / copying.

> - The above being said, we should not regress workloads that have
>   writeback disabled, so we either need to keep the pages in the
>   swapcache to avoid the extra allocations/copies -- or avoid storing
>   the pages in zswap completely if writeback is disabled. If writeback
>   is disabled and the page is incompressible, we could probably just put
>   it in the unevictable LRU because that's what it really is. We'd need
>   to make sure we remove it when it becomes compressible again. The
>   first approach is probably simpler.

This is a good point. While I don't have the numbers to back this up, I have
an intuition that this patch really sees the most benefits from reducing
CPU time spent trying to compress incompressible pages, rather than from
maintaining a better LRU ordering. For that reason I also suspect that we
see much better performance gains when writeback is disabled. Following that
logic... maybe there is some future work to be done that just moves these
incompressible pages to an unevictable LRU when writeback is disabled?

For now, I'm still experimenting with keeping the page in swapcache. I'll
be sure to report back with cool findings! Thank you again for your review
of this idea Yosry, I hope you have a great day!
Joshua

Powered by blists - more mailing lists