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

[Hao Jia <jiahao.kernel@gmail.com>]

On 2026/7/17 08:08, Yosry Ahmed wrote:
> On Thu, Jul 16, 2026 at 11:19 AM Nhat Pham <nphamcs@gmail.com> wrote:
>>
>> On Wed, Jul 15, 2026 at 7:21 PM Hao Jia <jiahao.kernel@gmail.com> wrote:
>>>
>>>
>>>
>>> On 2026/7/16 00:13, Yosry Ahmed wrote:
>>>> On Wed, Jul 15, 2026 at 5:31 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2026/7/15 10:31, Andrew Morton wrote:
>>>>>> On Tue, 14 Jul 2026 09:52:59 -0700 Yosry Ahmed <yosry@kernel.org> wrote:
>>>>>>
>>>>>>>> When memory cgroup is disabled, mem_cgroup_iter() always returns NULL.
>>>>>>>> Therefore, the global shrinker shrink_worker() always takes the !memcg
>>>>>>>> branch. After MAX_RECLAIM_RETRIES empty walks, the worker simply gives up,
>>>>>>>> so it fails to write back anything.
>>>>>>>>
>>>>>>>> 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. The
>>>>>>>> loop then safely bails out via the zswap_total_pages() <= thr 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@vger.kernel.org
>>>>>>>> Suggested-by: Nhat Pham <nphamcs@gmail.com>
>>>>>>>> Acked-by: Nhat Pham <nphamcs@gmail.com>
>>>>>>>> Acked-by: Yosry Ahmed <yosry@kernel.org>
>>>>>>>> Reported-by: Yosry Ahmed <yosry@kernel.org>
>>>>>>>> Closes: https://lore.kernel.org/all/CAO9r8zPVzMKFbCixxD-qgtRrkFxWVrHiZZeLc=eyTPKPVQgX4g@mail.gmail.com
>>>>>>>> Signed-off-by: Hao Jia <jiahao1@lixiang.com>
>>>>>>>
>>>>>>> Patch 2 doesn't really depend on this one, right?
>>>>>>>
>>>>>>> If that's the case I think this can (and should be) picked up
>>>>>>> separately as a hotfix. Andrew, WDYT?
>>>>>>
>>>>>> Please update the changelog to clearly describe the userspace-visible
>>>>>> effects of the bug, thanks.
>>>>>
>>>>> I am not entirely sure if my understanding is correct here, but maybe I
>>>>> should add something like this to the commit message?
>>>>>
>>>>> When cgroup_disable=memory is used (or with CONFIG_MEMCG=n), the global
>>>>> shrinker fails to write back any pages. Consequently, the zswap pool
>>>>> fills up to its limit and rejects further storage, preventing memory
>>>>> pressure from being offloaded to the backing swap device.
>>>>
>>>> I think you can simply write that zswap writeback when the limit is
>>>> hit is broken when memcg is disabled.
>>> Will do. Thanks!
>>>
>>>>
>>>>>
>>>>>> Also, AI review has flagged several possible issues, all appear to be
>>>>>> serious:
>>>>>>         https://sashiko.dev/#/patchset/20260714081510.16895-1-jiahao.kernel@gmail.com
>>>>>
>>>>> For AI review comments on this patch:
>>>>> I suspect this scenario might only exist in theory. For zswap LRU to be
>>>>> empty while zswap_total_pages() > thr holds true, it would require a
>>>>> prolonged state where there are always more than thr zswap entries on
>>>>> the zswap LRU whenever zswap_total_pages() > thr is evaluated, yet the
>>>>> zswap LRU happens to be empty during shrink_memcg(root_memcg).
>>>>>
>>>>> If we want to fix this, perhaps we could do something like this?
>>>>>
>>>>> Yosry, Nhat, what are your thoughts on this?
>>>>
>>>> Do we need to do this? The last paragraph in your changelog explains
>>>> why this can't happen because zswap_total_pages() should be 0 in this
>>>> case. Did I miss something?
>>>
>>> The loop would require the following sequence to repeat indefinitely:
>>>
>>> 1、zswap_total_pages() > thr evaluates to true.
>>> 2、During shrink_memcg(root_memcg), the zswap LRU is concurrently drained
>>> to empty.
>>> 3、Before zswap_total_pages() > thr is evaluated again, the zswap LRU is
>>> heavily refilled such that zswap_total_pages() > thr holds true once more.
>>>
>>> For our case to manifest, it would require zswap_total_pages() and the
>>> zswap LRU state to repeatedly hit this exact window with perfect
>>> alignment over a **prolonged period**. Therefore, I suspect this
>>> scenario might only exist in theory.
>>
>> Yeah seems very artificial indeed.
>>
>> If we want to be extra careful here, maybe we can put a cond_resched()
>> there or replace the continue with goto resched or sth?
> 
> I think it's also possible with the current code with memcg enabled.
> It's still possible that the shrinker puts usage under the acceptance
> threshold and then a new zswap stores puts it back above the
> threshold, and so on.
> 
> Perhaps as Nhat said, just goto resched instead of continue if we're
> really worried, but I think even that is not really necessary.
> 

Yes, but to be strictly rigorous, let's add a goto resched; anyway.

Thanks,
Hao

> I think we generally want to clean up and simplify the shrinking loop
> in zswap_shrinker(), but I don't have any great ideas.