Re: [RFC PATCH v2] mm/zswap: store <PAGE_SIZE compression failed page as-is

[SeongJae Park <sj@kernel.org>]

On Wed, 6 Aug 2025 12:32:21 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:

> Hi SJ,
> 
> Overall this looks good to me. On top of the feedback provided by
> others, I have a few comments below.
> 
> On Mon, Aug 04, 2025 at 05:29:54PM -0700, SeongJae Park wrote:
> > @@ -142,6 +142,15 @@ User can enable it as follows::
> >  This can be enabled at the boot time if ``CONFIG_ZSWAP_SHRINKER_DEFAULT_ON`` is
> >  selected.
> >  
> > +If a page cannot be compressed into a size smaller than PAGE_SIZE, it can be
> > +beneficial to save the content as is without compression, to keep the LRU
> > +order.  Users can enable this behavior, as follows::
> > +
> > +  echo Y > /sys/module/zswap/parameters/save_incompressible_pages
> > +
> > +This is disabled by default, and doesn't change behavior of zswap writeback
> > +disabled case.
> > +
> >  A debugfs interface is provided for various statistic about pool size, number
> >  of pages stored, same-value filled pages and various counters for the reasons
> >  pages are rejected.
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 7e02c760955f..6e196c9a4dba 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -129,6 +129,11 @@ static bool zswap_shrinker_enabled = IS_ENABLED(
> >  		CONFIG_ZSWAP_SHRINKER_DEFAULT_ON);
> >  module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644);
> >  
> > +/* Enable/disable incompressible pages storing */
> > +static bool zswap_save_incompressible_pages;
> > +module_param_named(save_incompressible_pages, zswap_save_incompressible_pages,
> > +		bool, 0644);
> 
> Please remove the knob and just make it the default behavior.
> 
> With writeback enabled, the current behavior is quite weird:
> compressible pages to into zswap, then get written to swap in LRU
> order. Incompressible pages get sent to swap directly. This is an
> obvious age inversion, and the performance problems this has caused
> was a motivating factor for the ability to disable writeback.
> 
> I don't think there is much point in keeping that as an officially
> supported mode.

Makes sense, I agree!  I will do so in the next version.

> 
> > @@ -937,6 +942,29 @@ static void acomp_ctx_put_unlock(struct crypto_acomp_ctx *acomp_ctx)
> >  	mutex_unlock(&acomp_ctx->mutex);
> >  }
> >  
> > +/*
> > + * Determine whether to save given page as-is.
> > + *
> > + * If a page cannot be compressed into a size smaller than PAGE_SIZE, it can be
> > + * beneficial to saving the content as is without compression, to keep the LRU
> > + * order.  This can increase memory overhead from metadata, but in common zswap
> > + * use cases where there are sufficient amount of compressible pages, the
> > + * overhead should be not critical, and can be mitigated by the writeback.
> > + * Also, the decompression overhead is optimized.
> > + *
> > + * When the writeback is disabled, however, the additional overhead could be
> > + * problematic.  For the case, just return the failure.  swap_writeout() will
> > + * put the page back to the active LRU list in the case.
> > + */
> > +static bool zswap_save_as_is(int comp_ret, unsigned int dlen,
> > +		struct page *page)
> > +{
> > +	return zswap_save_incompressible_pages &&
> > +			(comp_ret || dlen == PAGE_SIZE) &&
> > +			mem_cgroup_zswap_writeback_enabled(
> > +					folio_memcg(page_folio(page)));
> > +}
> > +
> >  static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> >  			   struct zswap_pool *pool)
> >  {
> 
> > @@ -1001,6 +1034,17 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> >  	return comp_ret == 0 && alloc_ret == 0;
> >  }
> >  
> > +/*
> > + * If save_incompressible_pages is set and writeback is enabled, incompressible
> > + * pages are saved as is without compression.  For more details, refer to the
> > + * comments of zswap_save_as_is().
> > + */
> > +static bool zswap_saved_as_is(struct zswap_entry *entry, struct folio *folio)
> > +{
> > +	return entry->length == PAGE_SIZE && zswap_save_incompressible_pages &&
> > +		mem_cgroup_zswap_writeback_enabled(folio_memcg(folio));
> > +}
> 
> I don't think there will be much left of these helpers once you
> incorporate Nhat's feedback, but please open-code these in either
> case. They're single user, hide what's going on, and the similar names
> doesn't do them any favors.

Agreed, I will do open-code these in the next version.


Thanks,
SJ