Date: Wed, 29 May 2024 21:42:23 +0900
From: Takero Funaki <flintglass@...il.com>
To: Nhat Pham <nphamcs@...il.com>
Cc: Johannes Weiner <hannes@...xchg.org>, Yosry Ahmed <yosryahmed@...gle.com>, 
	Chengming Zhou <chengming.zhou@...ux.dev>, Jonathan Corbet <corbet@....net>, 
	Andrew Morton <akpm@...ux-foundation.org>, 
	Domenico Cerasuolo <cerasuolodomenico@...il.com>, linux-mm@...ck.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] mm: zswap: fix global shrinker memcg iteration

2024年5月29日(水) 0:10 Nhat Pham <nphamcs@...ilcom>:
>
> On Mon, May 27, 2024 at 9:34 PM Takero Funaki <flintglass@...il.com> wrote:
> >
> > This patch fixes an issue where the zswap global shrinker stopped
> > iterating through the memcg tree.
>
> Did you observe this problem in practice?

Thank you for your comments.

I think this situation is rare, but I need to address this to ensure
patch 2 will not stop at the offline memcg.
The main issue I am seeing in version 6.9.0 to 6.10.0rc1 is that the
shrinker did not shrink until accept_thr_percent, and the
written_back_pages is smaller than max_limit_hit.
This can be explained by the shrinker stopping too early or looping
over only part of the tree.

> >
> > The problem was that `shrink_worker()` would stop iterating when a memcg
> > was being offlined and restart from the tree root.  Now, it properly
> > handles the offlining memcg and continues shrinking with the next memcg.
> >
> > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware")
> > Signed-off-by: Takero Funaki <flintglass@...il.com>
> > ---
> >  mm/zswap.c | 76 ++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 56 insertions(+), 20 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index a50e2986cd2f..0b1052cee36c 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -775,12 +775,27 @@ void zswap_folio_swapin(struct folio *folio)
> >         }
> >  }
> >
> > +/*
> > + * This function should be called when a memcg is being offlined.
> > + *
> > + * Since the global shrinker shrink_worker() may hold a reference
> > + * of the memcg, we must check and release the reference in
> > + * zswap_next_shrink.
> > + *
> > + * shrink_worker() must handle the case where this function releases
> > + * the reference of memcg being shrunk.
> > + */
> >  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg)
> >  {
> >         /* lock out zswap shrinker walking memcg tree */
> >         spin_lock(&zswap_shrink_lock);
> > -       if (zswap_next_shrink == memcg)
> > -               zswap_next_shrink = mem_cgroup_iter(NULL, zswap_next_shrink, NULL);
> > +
> > +       if (READ_ONCE(zswap_next_shrink) == memcg) {
> > +               /* put back reference and advance the cursor */
> > +               memcg = mem_cgroup_iter(NULL, memcg, NULL);
> > +               WRITE_ONCE(zswap_next_shrink, memcg);
> > +       }
>
> Hmm could you expand on how your version differs from what's existing?
> What's the point of all these extra steps? The whole thing is done
> under a big spin lock anyway.

I agree that the code is not necessary. These ONCE are for clarifying
what is expected and to align with shrink_worker(), where READ_ONCE is
required to reload the shared variable.
It can be a safeguard for me, sometimes forgetting that we must not
read zswap_next_shrink before acquiring the lock.

> > +
> >         spin_unlock(&zswap_shrink_lock);
> >  }
> >
> > @@ -1312,25 +1327,38 @@ static int shrink_memcg(struct mem_cgroup *memcg)
> >
> >  static void shrink_worker(struct work_struct *w)
> >  {
> > -       struct mem_cgroup *memcg;
> > +       struct mem_cgroup *memcg = NULL;
> > +       struct mem_cgroup *next_memcg;
> >         int ret, failures = 0;
> >         unsigned long thr;
> >
> >         /* Reclaim down to the accept threshold */
> >         thr = zswap_accept_thr_pages();
> >
> > -       /* global reclaim will select cgroup in a round-robin fashion. */
> > +       /* global reclaim will select cgroup in a round-robin fashion.
> > +        *
> > +        * We save iteration cursor memcg into zswap_next_shrink,
> > +        * which can be modified by the offline memcg cleaner
> > +        * zswap_memcg_offline_cleanup().
> > +        */
>
> I feel like the only difference between this loop and the old loop, is
> that if we fail to get an online reference to memcg, we're trying from
> the next one (given by mem_cgroup_iter()) rather than from the top
> (NULL).
>
> For instance, consider the first two steps:
>
> 1. First, we check if memcg is the same as zswap_next_shrink. if not,
> then reset it to zswap_next_shrink.
> 2. Advance memcg, then store the result at zswap_next_shrink.
>
> Under the big zswap_shrink_lock, this is the same as:
>
> 1. Advance zswap_next_shrink.
> 2. Look up zswap_next_shrink, then store it under the local memcg variable.
>
> which is what we were previously doing.
>
> The next step is shared - check for null memcg (which again, is the
> same as zswap_next_shrink now), and attempt to get an online
> reference.
> The only difference is when we fail to get the online reference -
> instead of starting from the top, we advance memcg again.
>
> Can't we just drop the lock, then add a continue statement? That will
> reacquire the lock, advance zswap_next_shrink, look up the result and
> store it at memcg, which is what you're trying to achieve?
>

If I understand correctly, in this offlining pattern, we are not
allowed to leave an offline memcg in zswap_next_shrink more than once.
While offline memcg can appear in memcg_iter_next repeatedly, the
cleaner is called only once per an offline memcg. (or is there some
retry logic?)

If the shrink_worker finds an offline memcg in iteration AFTER the
cleaner was called, we must put back the offline memcg reference
inside shrink_worker() BEFORE going to return/sleep.
Otherwise, the offline memcg reference will be kept in
zswap_next_shrink until the next shrink_worker() is requeued. There is
no rescue chance from the cleaner again.

This means the shrink_worker has to check:
1. We get a lock. Check if the memcg is online while locked.
2a. If online, it can be offlined while we have the lock. But the lock
assures us that the cleaner is waiting for the lock just behind us. We
can rely on this.
2b. If offline, we cannot determine if the cleaner has already been
called or is being called. We have to put back the offline memcg
reference, assuming no cleaner is available.

If we get offline memcg AND abort the shrinker by the max failure
limit, that breaks condition 2b. Thus we need to unconditionally
restart from the beginning of the do block.
I will add these expected situations to the comments.

For unlocking, should we rewrite,

goto iternext;

to

spin_unlock();
goto before_spin_lock; /* just after do{ */

I think that will minimize the critical section and satisfy the condition 2b.

For the memcg iteration,
> 2. Advance memcg, then store the result at zswap_next_shrink.
should be done only if `(memcg == zswap_next_shrink)`.

Say iterating A -> B -> C and A is being offlined.
While we have memcg=A and drop the lock, the cleaner may advance
zswap_next_shrink=A to B.
If we do not check `memcg != next_memcg`, we advance
zswap_next_shrink=B to C again, forgetting B.

That is the reason I added  `(memcg != next_memcg)` check.
Although It can be negligible as it only ignores one memcg per one
cleaner callback.

This is my understanding. Am I missing any crucial points? Any
comments or advice would be greatly appreciated.

> >         do {
> >                 spin_lock(&zswap_shrink_lock);
> > -               zswap_next_shrink = mem_cgroup_iter(NULL, zswap_next_shrink, NULL);
> > -               memcg = zswap_next_shrink;
> > +               next_memcg = READ_ONCE(zswap_next_shrink);
> > +
> > +               if (memcg != next_memcg) {
> > +                       /*
> > +                        * Ours was released by offlining.
> > +                        * Use the saved memcg reference.
> > +                        */
> > +                       memcg = next_memcg;
> > +               } else {
> > +iternext:
> > +                       /* advance cursor */
> > +                       memcg = mem_cgroup_iter(NULL, memcg, NULL);
> > +                       WRITE_ONCE(zswap_next_shrink, memcg);
> > +               }
> >
> >                 /*
> > -                * We need to retry if we have gone through a full round trip, or if we
> > -                * got an offline memcg (or else we risk undoing the effect of the
> > -                * zswap memcg offlining cleanup callback). This is not catastrophic
> > -                * per se, but it will keep the now offlined memcg hostage for a while.
> > -                *
>
> Why are we removing this comment?

The existing comment about aborting the loop on the offlining memcg,
is not valid on  this patch. The offline memcg will just be skipped.
I think the new behavior is commented at the beginning of the loop and
in the !mem_cgroup_tryget_online branch. Please let me know if you
have any suggestions.


> >                  * Note that if we got an online memcg, we will keep the extra
> >                  * reference in case the original reference obtained by mem_cgroup_iter
> >                  * is dropped by the zswap memcg offlining callback, ensuring that the
> > @@ -1345,16 +1373,18 @@ static void shrink_worker(struct work_struct *w)
> >                 }
> >
> >                 if (!mem_cgroup_tryget_online(memcg)) {
> > -                       /* drop the reference from mem_cgroup_iter() */
> > -                       mem_cgroup_iter_break(NULL, memcg);
> > -                       zswap_next_shrink = NULL;
> > -                       spin_unlock(&zswap_shrink_lock);
> > -
> > -                       if (++failures == MAX_RECLAIM_RETRIES)
> > -                               break;
> > -
> > -                       goto resched;
>
> I think we should still increment the failure counter, to guard
> against long running/infinite loops.

Since we skip the offline memcg instead of restarting from the root,
the new iteration will be terminated when it reaches tree root in
finite steps.  I am afraid that counting this as a failure will
terminate the shrinker too easily.

I think shrinker_worker() running longer is not an issue because the
amount of work per the while loop is limited by rescheduling. As it
has a dedicated WQ_MEM_RECLAIM workqueue, returning from the function
is not necessary. cond_resched() should allow the other workqueue to
run.
The next patch also adds a progress check per walking.


> > +                       /*
> > +                        * It is an offline memcg which we cannot shrink
> > +                        * until its pages are reparented.
> > +                        * Put back the memcg reference before cleanup
> > +                        * function reads it from zswap_next_shrink.
> > +                        */
> > +                       goto iternext;
> >                 }
> > +               /*
> > +                * We got an extra memcg reference before unlocking.
> > +                * The cleaner cannot free it using zswap_next_shrink.
> > +                */
> >                 spin_unlock(&zswap_shrink_lock);
> >
> >                 ret = shrink_memcg(memcg);
> > @@ -1368,6 +1398,12 @@ static void shrink_worker(struct work_struct *w)
> >  resched:
> >                 cond_resched();
> >         } while (zswap_total_pages() > thr);
> > +
> > +       /*
> > +        * We can still hold the original memcg reference.
> > +        * The reference is stored in zswap_next_shrink, and then reused
> > +        * by the next shrink_worker().
> > +        */
> >  }
> >
> >  /*********************************
> > --
> > 2.43.0
> >



--

<flintglass@...il.com>

Powered by blists - more mailing lists