linux-kernel - Re: [PATCH v3 2/2] mm: zswap: fix global shrinker error handling logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPpoddcw7BD13ME8xG5TP=kKV=5t_JCxA0DW3t7C5o1wkC5tfg@mail.gmail.com>
Date: Fri, 26 Jul 2024 17:54:36 +0900
From: Takero Funaki <flintglass@...il.com>
To: Chengming Zhou <chengming.zhou@...ux.dev>
Cc: Nhat Pham <nphamcs@...il.com>, Johannes Weiner <hannes@...xchg.org>, 
	Yosry Ahmed <yosryahmed@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] mm: zswap: fix global shrinker error handling logic

Thanks for your comments.


2024年7月26日(金) 12:21 Chengming Zhou <chengming.zhou@...ux.dev>:
> > and, the reasons to (not) increment the progress:
> >
> > @@ -1387,10 +1407,20 @@ static void shrink_worker(struct work_struct *w)
> >                  /* drop the extra reference */
> >                  mem_cgroup_put(memcg);
> >
> > -               if (ret == -EINVAL)
> > -                       break;
> > +               /*
> > +                * There are no writeback-candidate pages in the memcg.
> > +                * This is not an issue as long as we can find another memcg
> > +                * with pages in zswap. Skip this without incrementing progress
> > +                * and failures.
> > +                */
> > +               if (ret == -ENOENT)
> > +                       continue;
> > +
> >                  if (ret && ++failures == MAX_RECLAIM_RETRIES)
> >                          break;
> > +
> > +               /* completed writeback or incremented failures */
> > +               ++progress;
>
> Maybe the name "progress" is a little confusing here? "progress" sounds
> to me that we have some writeback completed.
>
> But actually it just means we have encountered some candidates, right?
>
> Thanks.
>
>

Yes, the `++progress` counts both error and success as an iteration
progress for valid memcgs (not writeback amount). Incrementing only on
success will overly increment failures counter if there is only one
memcg, one from writeback failure and one from tree walk ends, the
worker aborts on 8 failures instead of 16.
`++candidates;` would be better? replacing the name and fixing commit
messages for v4.