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

[Yosry Ahmed]

> > 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.

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?

>
>   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.