Received-SPF: pass (google.com: domain of linux-kernel+bounces-194122-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240528043404.39327-2-flintglass@gmail.com> <20240528043404.39327-4-flintglass@gmail.com>
 <CAKEwX=NnigYafFtQ63mknr_Gi7AVG5rp514GHi0a1K5PiNUxSQ@mail.gmail.com>
In-Reply-To: <CAKEwX=NnigYafFtQ63mknr_Gi7AVG5rp514GHi0a1K5PiNUxSQ@mail.gmail.com>
From: Takero Funaki <flintglass@gmail.com>
Date: Wed, 29 May 2024 22:00:02 +0900
Message-ID: <CAPpodddOjhqh27Nt_Gkf=BTG0NBvWR4sxLPWQRHmPmwGOk0JAA@mail.gmail.com>
Subject: Re: [PATCH 2/3] mm: zswap: fix global shrinker error handling logic
To: Nhat Pham <nphamcs@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Yosry Ahmed <yosryahmed@google.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Jonathan Corbet <corbet@lwn.net>, 
	Andrew Morton <akpm@linux-foundation.org>, 
	Domenico Cerasuolo <cerasuolodomenico@gmail.com>, linux-mm@kvack.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2024=E5=B9=B45=E6=9C=8829=E6=97=A5(=E6=B0=B4) 0:11 Nhat Pham <nphamcs@gmail=
com>:
>
> On Mon, May 27, 2024 at 9:34=E2=80=AFPM Takero Funaki <flintglass@gmail.c=
om> wrote:
> >
> > This patch fixes zswap global shrinker that did not shrink zpool as
> > expected.
> >
> > The issue it addresses is that `shrink_worker()` did not distinguish
> > between unexpected errors and expected error codes that should be
> > skipped, such as when there is no stored page in a memcg. This led to
> > the shrinking process being aborted on the expected error codes.
> >
> > The shrinker should ignore these cases and skip to the next memcg.
> > However,  skipping all memcgs presents another problem. To address this=
,
> > this patch tracks progress while walking the memcg tree and checks for
> > progress once the tree walk is completed.
> >
> > To handle the empty memcg case, the helper function `shrink_memcg()` is
> > modified to check if the memcg is empty and then return -ENOENT.
> >
> > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware")
> > Signed-off-by: Takero Funaki <flintglass@gmail.com>
> > ---
> >  mm/zswap.c | 31 ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 0b1052cee36c..08a6f5a6bf62 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1304,7 +1304,7 @@ static struct shrinker *zswap_alloc_shrinker(void=
)
> >
> >  static int shrink_memcg(struct mem_cgroup *memcg)
> >  {
> > -       int nid, shrunk =3D 0;
> > +       int nid, shrunk =3D 0, stored =3D 0;
> >
> >         if (!mem_cgroup_zswap_writeback_enabled(memcg))
> >                 return -EINVAL;
> > @@ -1319,9 +1319,16 @@ static int shrink_memcg(struct mem_cgroup *memcg=
)
> >         for_each_node_state(nid, N_NORMAL_MEMORY) {
> >                 unsigned long nr_to_walk =3D 1;
> >
> > +               if (!list_lru_count_one(&zswap_list_lru, nid, memcg))
> > +                       continue;
> > +               ++stored;
> >                 shrunk +=3D list_lru_walk_one(&zswap_list_lru, nid, mem=
cg,
> >                                             &shrink_memcg_cb, NULL, &nr=
_to_walk);
> >         }
> > +
> > +       if (!stored)
> > +               return -ENOENT;
> > +
> >         return shrunk ? 0 : -EAGAIN;
> >  }
> >
> > @@ -1329,12 +1336,18 @@ static void shrink_worker(struct work_struct *w=
)
> >  {
> >         struct mem_cgroup *memcg =3D NULL;
> >         struct mem_cgroup *next_memcg;
> > -       int ret, failures =3D 0;
> > +       int ret, failures =3D 0, progress;
> >         unsigned long thr;
> >
> >         /* Reclaim down to the accept threshold */
> >         thr =3D zswap_accept_thr_pages();
> >
> > +       /*
> > +        * We might start from the last memcg.
> > +        * That is not a failure.
> > +        */
> > +       progress =3D 1;
> > +
> >         /* global reclaim will select cgroup in a round-robin fashion.
> >          *
> >          * We save iteration cursor memcg into zswap_next_shrink,
> > @@ -1366,9 +1379,12 @@ static void shrink_worker(struct work_struct *w)
> >                  */
> >                 if (!memcg) {
> >                         spin_unlock(&zswap_shrink_lock);
> > -                       if (++failures =3D=3D MAX_RECLAIM_RETRIES)
> > +
> > +                       /* tree walk completed but no progress */
> > +                       if (!progress && ++failures =3D=3D MAX_RECLAIM_=
RETRIES)
> >                                 break;
> >
> > +                       progress =3D 0;
> >                         goto resched;
> >                 }

Here, the `progress` counter tracks how many memcgs successfully evict
a page in a tree walking. (not per the while loop) then reset to 0.
progress > 0 ensures there is progress.
If we visit the tree root (NULL) without any progress, it will be a failure=
.

Before the loop starts, progress counter is initialized to 1 because
the first tree walk might not  iterate all the memcgs, e.g. the
previous worker was terminated at the very last memcg.


> >
> > @@ -1391,10 +1407,15 @@ static void shrink_worker(struct work_struct *w=
)
> >                 /* drop the extra reference */
> >                 mem_cgroup_put(memcg);
> >
> > -               if (ret =3D=3D -EINVAL)
> > -                       break;
> > +               /* not a writeback candidate memcg */
> > +               if (ret =3D=3D -EINVAL || ret =3D=3D -ENOENT)
> > +                       continue;
> > +
>
> Can we get into an infinite loop or a really long running loops here
> if all memcgs have their writeback disabled?
>
> >                 if (ret && ++failures =3D=3D MAX_RECLAIM_RETRIES)
> >                         break;
> > +
> > +               ++progress;
> > +               /* reschedule as we performed some IO */
> >  resched:
> >                 cond_resched();
> >         } while (zswap_total_pages() > thr);
> > --
> > 2.43.0
> >


--=20

<flintglass@gmail.com>