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

[Nhat Pham]

On Wed, Jul 15, 2026 at 7:21 PM Hao Jia <jiahao.kernel@xxxxxxxxx> wrote:
>
>
>
> On 2026/7/16 00:13, Yosry Ahmed wrote:
> > On Wed, Jul 15, 2026 at 5:31 AM Hao Jia <jiahao.kernel@xxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2026/7/15 10:31, Andrew Morton wrote:
> >>> On Tue, 14 Jul 2026 09:52:59 -0700 Yosry Ahmed <yosry@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >>>>> Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> >>>>> Acked-by: Nhat Pham <nphamcs@xxxxxxxxx>
> >>>>> Acked-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> >>>>> Reported-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> >>>>> Closes: https://lore.kernel.org/all/CAO9r8zPVzMKFbCixxD-qgtRrkFxWVrHiZZeLc=eyTPKPVQgX4g@xxxxxxxxxxxxxx
> >>>>> Signed-off-by: Hao Jia <jiahao1@xxxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxx
> >>
> >> 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?