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

[Hao Jia]

On 2026/6/30 02:37, Nhat Pham wrote:
> On Mon, Jun 29, 2026 at 4:20 AM Hao Jia <jiahao.kernel@xxxxxxxxx> wrote:
> > From: Hao Jia <jiahao1@xxxxxxxxxxx>
> >
> > 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. Stop the loop once
> > shrink_memcg() reports -ENOENT, since the root LRU is the only target and
> > -ENOENT means it has been exhausted.
> >
> > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/CAO9r8zPVzMKFbCixxD-qgtRrkFxWVrHiZZeLc=eyTPKPVQgX4g@xxxxxxxxxxxxxx
> > Signed-off-by: Hao Jia <jiahao1@xxxxxxxxxxx>
>
> Ah good catch.
>
>
>
> > ---
> >   mm/zswap.c | 16 ++++++++++++++--
> >   1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 761cd699e0a3..0f8f04f22888 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1356,7 +1356,12 @@ static void shrink_worker(struct work_struct *w)
> >                  } while (memcg && !mem_cgroup_tryget_online(memcg));
> >                  spin_unlock(&zswap_shrink_lock);
> >
> > -               if (!memcg) {
> > +               /*
> > +                * Reaching a NULL memcg means a full hierarchy pass completed.
> > +                * Exclude the memcg-disabled case, where it is always NULL, and
> > +                * fall through to shrink the root LRU directly.
> > +                */
> > +               if (!memcg && !mem_cgroup_disabled()) {
> >                          /*
> >                           * Continue shrinking without incrementing failures if
> >                           * we found candidate memcgs in the last tree walk.
>
> nit: I wonder if we can just merge this comment with the new comment
> you just added.

Updated. Please see below.

> > @@ -1378,8 +1383,15 @@ static void shrink_worker(struct work_struct *w)
> >                   * with pages in zswap. Skip this without incrementing attempts
> >                   * and failures.
> >                   */
> > -               if (ret == -ENOENT)
> > +               if (ret == -ENOENT) {
> > +                       /*
> > +                        * With memcg disabled the root LRU is the only target, so
> > +                        * we should abort if it has no writeback-candidate pages.
> > +                        */
> > +                       if (mem_cgroup_disabled())
> > +                               break;
>
> Hmm do we need to do this? Consider a system with cgroup enabled but
> with just one cgroup (root?). The behavior would just be trying that
> cgroup for MAX_RECLAIM_RETRIES failure attempts, correct?
>
> In that case, we don't need to do this check, and we would get the
> same behavior. The loop would terminate after MAX_RECLAIM_RETRIES :)
>
> Could you fact-check me? :)

Exactly. When memcg is disabled, shrink_memcg() returns -ENOENT only if 
the root LRU is empty. An empty root LRU implies that the total pages 
have already dropped below the threshold (thr). At this point, the loop 
safely terminates because of the zswap_total_pages() <= thr check. In 
all other cases (where shrink_memcg() returns anything other than 
-ENOENT), the loop will eventually exit either by hitting the 
MAX_RECLAIM_RETRIES limit or when zswap_total_pages() <= thr.

How about something like this? If there are no objections, I'll fold 
this into the next version.

    mm/zswap: Fix global shrinker when memory cgroup is disabled

    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
    Reported-by: Yosry Ahmed <yosry@xxxxxxxxxx>
    Closes: 
https://lore.kernel.org/all/CAO9r8zPVzMKFbCixxD-qgtRrkFxWVrHiZZeLc=eyTPKPVQgX4g@xxxxxxxxxxxxxx
    Signed-off-by: Hao Jia <jiahao1@xxxxxxxxxxx>

diff --git a/mm/zswap.c b/mm/zswap.c
index 4b5149173b0e..9d4f19fc440e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1361,11 +1361,12 @@ static void shrink_worker(struct work_struct *w)
                } while (memcg && !mem_cgroup_tryget_online(memcg));
                spin_unlock(&zswap_shrink_lock);

-               if (!memcg) {
-                       /*
-                        * Continue shrinking without incrementing 
failures if
-                        * we found candidate memcgs in the last tree walk.
-                        */
+               /*
+                * A NULL memcg ends a full hierarchy pass (except when 
memcg is
+                * disabled, where it is always NULL: fall through to 
the root LRU).
+                * Count a failure only if the pass found no candidates.
+                */
+               if (!memcg && !mem_cgroup_disabled()) {
                        if (!attempts && ++failures == MAX_RECLAIM_RETRIES)
                                break;

Thanks,
Hao