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

[Yosry Ahmed]

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.

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