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

[Hao Jia]

On 2026/7/30 08:30, Yosry Ahmed wrote:
> 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.

Yeah, checkpatch will complain if it's missing.

> > 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 we scan and attempt to write back SWAP_CLUSTER_MAX zswap entries per 
node, it might lead to excessive writeback on machines with many NUMA 
nodes. Furthermore, I'm concerned about introducing higher latency in 
synchronous shrink paths like zswap_store()—especially on systems with a 
large number of NUMA nodes, where it could end up writing back hundreds 
of pages in a single call.

Maybe we could do something like this instead? That way, in the 
worst-case scenario, it falls back to the baseline behavior without 
introducing any extra latency risks.

 static int shrink_memcg(struct mem_cgroup *memcg)
 {
-       int nid, shrunk = 0, scanned = 0;
+       unsigned long node_batch, scanned = 0;
+       int nid, shrunk = 0;

        if (!mem_cgroup_zswap_writeback_enabled(memcg))
                return -ENOENT;
@@ -1289,14 +1313,26 @@ static int shrink_memcg(struct mem_cgroup *memcg)
        if (memcg && !mem_cgroup_online(memcg))
                return -ENOENT;

+       node_batch = max(1UL, SWAP_CLUSTER_MAX / 
num_node_state(N_NORMAL_MEMORY));
        for_each_node_state(nid, N_NORMAL_MEMORY) {
-               unsigned long nr_to_walk = 1;
+               unsigned long nr_to_walk, budget;
+
+               /*
+                * Cap the scan at the per-node LRU length so each entry is
+                * scanned at most once per call.
+                */
+               budget = min(node_batch,
+                            list_lru_count_one(&zswap_list_lru, nid, 
memcg));
+               if (!budget)
+                       continue;

+               nr_to_walk = budget;
                shrunk += list_lru_walk_one(&zswap_list_lru, nid, memcg,
                                            &shrink_memcg_cb, NULL, 
&nr_to_walk);
-               scanned += 1 - nr_to_walk;
+               scanned += budget - nr_to_walk;
        }

+       /* Nothing was scanned: every LRU under @memcg was empty. */
        if (!scanned)
                return -ENOENT;

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