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

[Hao Jia]

On 2026/7/17 08:08, Yosry Ahmed wrote:
> On Thu, Jul 16, 2026 at 11:19 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > 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?
>
> 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.