Re: [RFC PATCH 0/2] mm: zsmalloc: make shrinker compaction budget-aware

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/11/04 14:54), Nhat Pham wrote:
> On Wed, Nov 4, 2026 at 11:46 AM Chris Wiseman <chriswiseman@google.com> wrote:
> >
> > On Wed, Nov 4, 2026 at 9:57 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > >
> > > Is this precheck basically checking if we can free at least one zspage
> > > of the size class before proceeding? Yeah the number looks good -
> > > let's add that, in parallel!
> >
> > Yeah, it just checks if a bucket has enough wasted slots to free at
> > least one full page before taking the locks.  Sergey pointed out those
> 
> You meant full zspage here right? IIUC we keep the entire chain of
> zpages around until the entire zspage is gone?

Full zspage page, right.

> > two counters aren't atomic together so an unlocked check is just a
> > best-effort hint - if you and Sergey are good with that, happy to send a
> > patch.
> 
> Personally I'm happy with it being a best-effort hint!
> 
> We can also recheck after we acquire the lock (but I suspect it's
> unlikely we'll get radically different decision?)
> 
> Sergey, how do you feel about this?

Well, I guess... I don't mind the lockless best effort hint.

I suppose another way could be an introduction of ->can_compact bool flag,
which we can set under the lock in zs_free() (that's where fragmentation is
created).  zs_can_compact() doesn't seem too expensive, but calling it on
every zs_free() looks a bit bad, we sort of want to do less work in zs_free().

> >
> > > Ah ok hmm. Sounds like internal compaction isn't that helpful, i.e
> > > zsmalloc already does a pretty decent job at packing objects that
> > > there aren't a terrible amount of internal fragmentation in the first
> > > place...?
> > > Should we do CONFIG_ZSMALLOC_SHRINKER (default to N or to
> > > CONFIG_ZSWAP) for now, and remove it in a couple of releases if users
> > > in the wild do not complain?
> >
> > Compaction still helps when we trigger it between bursts (freed ~21 MB
> > before the swap storm and ~10 MB after).  It just wastes time running
> > mid-swap when new writes are filling those open slots right back up.
> > Defaulting CONFIG_ZSMALLOC_SHRINKER to N (or CONFIG_ZSWAP) for a couple
> > releases makes sense to me.
> 
> SGTM, but I'll let Sergey chime in!

So "depends on ZSWAP" looks a bit problematic, because zswap has both
ZSWAP_SHRINKER_DEFAULT_ON (which can select CONFIG_ZSMALLOC_SHRINKER),
but then zswap also has module_param_named(shrinker_enabled).  So if
want to tie zsmalloc to zswap shrinker then we probably need to wire
in that run-time knob as well.  If shrinker_enabled should imply zsmalloc
shrinker (which I think it didn't so far).  zswap folks, do you have any
preferences?