Re: [PATCH v3 1/2] mm/zswap: Fix global shrinker when memory cgroup is disabled

[Yosry Ahmed]

On Wed, Jul 29, 2026 at 3:58 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 29 Jul 2026 16:42:05 +0800 Hao Jia <jiahao.kernel@xxxxxxxxx> wrote:
>
> > Zswap writeback when the global pool limit is hit fails when memory
> > cgroup is disabled. The pool remains full until it is organically
> > drained by swapins or memory freeing, leading to zswap store failures
> > and pages bypassing getting written directly to the backing swap device,
> > causing LRU inversion (hotter pages with higher fault latency).
> >
> > This happens because mem_cgroup_iter() always returns NULL when
> > memory cgroups are disabled. As a result, the global shrinker
> > shrink_worker() repeatedly takes empty walks. After MAX_RECLAIM_RETRIES
> > failed attempts, the worker gives up without writing back any pages.
> >
> > Therefore, when memory cgroup is disabled, fall through with the !memcg
> > branch and shrink the root memcg directly.
> >
> > With memcg disabled, shrink_memcg() only returns -ENOENT when the root
> > LRU is empty, which means the total pages are already below thr. In the
> > absence of heavy concurrent zswap stores, the loop then safely bails out
> > via the zswap_total_pages() <= thr check; otherwise, it will resume
> > shrinking the memcg after processing the reschedule check. For any other
> > return value from shrink_memcg(), the loop is guaranteed to terminate,
> > either after MAX_RECLAIM_RETRIES failures or once the threshold is met.
> >
> > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware")
> > Cc: stable@xxxxxxxxxxxxxxx
>
> How does this affect users?  What behavior do they observe when it
> occurs?

I think the first paragraph sums it up pretty well, especially the
last sentence "hotter pages with higher fault latency".

>
> > Closes: https://lore.kernel.org/all/CAO9r8zPVzMKFbCixxD-qgtRrkFxWVrHiZZeLc=eyTPKPVQgX4g@xxxxxxxxxxxxxx
>
> hm, that isn't really a bug report and doesn't answer the above
> question.

Yeah, it isn't. Probably we should drop "Closes". I assume Hao added
it because checkpatch annoyingly complains if you add "Reported-by"
without "Closes", so Hao just linked to the thread where I pointed out
the bug.

>
> AI review asked a couple of questions:
>         https://sashiko.dev/#/patchset/20260729084206.77793-1-jiahao.kernel@xxxxxxxxx

The review on patch #1 is something theoretical, we discussed it at
length in previous versions.

For patch #2:

> Does this batching logic break NUMA fairness?
>
> Because for_each_node_state() always starts from the lowest node
> ID and breaks when the scan budget is exhausted, subsequent
> calls to shrink_memcg() will restart at the lowest node ID again.
>
> If the lowest node (typically Node 0) consistently has enough
> items to exhaust the scan budget, wouldn't we exclusively evict
> pages from it while ignoring older pages on other nodes? Could
> this cause LRU inversion across nodes, keeping older pages in
> memory on Node 1 while hot pages on Node 0 are evicted?

Yes, unfairness is possible.

For global shrinking, it's probably not an issue. We reclaim until we
hit the acceptance threshold and it's very unlikely this will happen
before iterating all nodes (given that the batch size is 32 pages).
However, with the shrink_memcg() path, we only reclaim one batch, so
there's a chance we'll always reclaim it from node 0.

Maybe we should just drop the early bailout and accept potentially
doing more writeback than needed. Hao, WDYT?

If you respin, please also drop the batch size argument to
shrink_memcg() as it's now always SWAP_CLUSTER_MAX.