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

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/[email protected]
> > >
> > > 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.