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

[Hao Jia]

On 2026/7/31 03:01, Johannes Weiner wrote:
> On Thu, Jul 30, 2026 at 10:48:38AM -0700, Yosry Ahmed wrote:
> > > > 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.
> >
> > We often ignore checkpatch if it's unreasonable.
> >
> > > > > 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.
> >
> > This basically errs on the side of honouring the batch size
> > (under-reclaim) instead of doing more writeback, right?
> >
> > I think I prefer erring on the side of doing more writeback, as it
> > would ultimately result in less LRU inversion, and we are already
> > doing writeback proactively during reclaim through the shrinker.
>
> Yes, LRU inversions are worse than reclaiming a few extra pages from
> the cold tail of the LRU. It's not cumulative after all, it just means
> we take a bigger bite and can take more stores before the next shrink.
>
> > The other option we can explore is keeping the existing logic with the
> > bailout, but start iterating the nodes at the folio's node in the
> > zswap_store() path. Basically pass an nid to shrink_memcg() and always
> > start there. This will avoid always reclaiming from node 0, and the
> > node of the folio being stored is more likely to be under pressure and
> > need the writeback. I am not sure how complex this would be and
> > whether it's worth doing.
> >
> > Johannes, Nhats, any thoughts on this?
>
> What happens if simply we do a hard SWAP_CLUSTER_MAX on each node?
>
> With cgroups, we put the zswap entry on the list_lru head of the node
> and memcg where the incoming page was.
>
> So if you're going after one specific cgroup, it's not like you
> *actually* reclaim 32 entries on each node of the system. You just
> write back from nodes where that cgroup has entries.
>
> The global shrinker also does one cgroup at a time (zswap_next_shrink)
> before checking the limit again, so that's fairly fine-grained too.
>
> Without cgroups, you could have zswap entries on all nodes. But if you
> have so many nodes that this would constitute hundreds of pages,
> presumably that's still a drop in the bucket compared to overall
> memory capacity. SWAP_CLUSTER_MAX isn't that big a number.
>
> So I don't think we need to get fancy. Just do SWAP_CLUSTER_MAX on
> each node. Delete that nr_to_walk carry-over between them.

Thanks for the input! I've posted the new implementation—please take a 
look when you have time.

https://lore.kernel.org/all/20260731071900.38942-1-jiahao.kernel@xxxxxxxxx

Thanks,
Hao