Message-ID: <aAd9G_BYEcNwVuSd@Asmaa.>
Date: Tue, 22 Apr 2025 04:27:23 -0700
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Joshua Hahn <joshua.hahnjy@...il.com>
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

On Wed, Apr 02, 2025 at 01:06:49PM -0700, Joshua Hahn wrote:
> On Mon, 16 Oct 2023 17:57:31 -0700 Yosry Ahmed <yosryahmed@...gle.com> wrote:
> 
> > On Mon, Oct 16, 2023 at 5:35 PM Nhat Pham <nphamcs@...il.com> wrote:
> 
> > 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.
> 
> Hi Yosry,
> 
> Apologies for reviving an old thread, but I wasn't sure whether opening an
> entirely new thread was a better choice : -)

This seems like the right choice to me :)

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

> 
> 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()?

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.

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

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.

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

  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.

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

> 
> Just wanted to put this here in case you were still thinking about this idea.
> What do you think? Ideally, there would be a way to keep the page around in
> the zswap LRU, but do not have to re-allocate a new page on a fault, but this
> seems like a bigger task.
> 
> Ultimately the goal is to prevent an incompressible page from hoarding the
> compression algorithm on multiple reclaim attempts, but if we are spending
> more time by allocating new pages... maybe this isn't the correct approach :(
> 
> Please let me know if you have any thoughts on this : -)
> Have a great day!
> Joshua
> 
> Sent using hkml (https://github.com/sjp38/hackermail)
> 
> 

Powered by blists - more mailing lists