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

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

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

https://lore.kernel.org/all/[email protected]

Thanks,
Hao