Received-SPF: pass (google.com: domain of linux-kernel+bounces-212055-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240608155316.451600-1-flintglass@gmail.com> <20240608155316.451600-2-flintglass@gmail.com>
 <CAKEwX=P1Ojb71AEJ2gzQTrfWidFPcJZmoNxEwji7TceBN-szCg@mail.gmail.com>
In-Reply-To: <CAKEwX=P1Ojb71AEJ2gzQTrfWidFPcJZmoNxEwji7TceBN-szCg@mail.gmail.com>
From: Takero Funaki <flintglass@gmail.com>
Date: Thu, 13 Jun 2024 03:16:42 +0900
Message-ID: <CAPpoddeigM44jhTA8Ua=+J4MC1MikouBZVoPrCW2LZF+9r5YeA@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] mm: zswap: fix global shrinker memcg iteration
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, Shakeel Butt <shakeel.butt@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2024=E5=B9=B46=E6=9C=8812=E6=97=A5(=E6=B0=B4) 3:26 Nhat Pham <nphamcs@gmail=
.com>:

>
> As I have noted in v0, I think this is unnecessary and makes it more conf=
using.
>

Does spin_lock() ensure that compiler optimizations do not remove
memory access to an external variable? I think we need to use
READ_ONCE/WRITE_ONCE for shared variable access even under a spinlock.
For example,
https://elixir.bootlin.com/linux/latest/source/mm/mmu_notifier.c#L234

isn't this a common use case of READ_ONCE?
```c
bool shared_flag =3D false;
spinlock_t flag_lock;

void somefunc(void) {
    for (;;) {
        spin_lock(&flag_lock);
        /* check external updates */
        if (READ_ONCE(shared_flag))
            break;
        /* do something */
        spin_unlock(&flag_lock);
    }
    spin_unlock(&flag_lock);
}
```
Without READ_ONCE, the check can be extracted from the loop by optimization=
.

In shrink_worker, zswap_next_shrink is the shared_flag , which can be
updated by concurrent cleaner threads, so it must be re-read every
time we reacquire the lock. Am I badly misunderstanding something?

> >         do {
> > +iternext:
> >                 spin_lock(&zswap_shrink_lock);
> > -               zswap_next_shrink =3D mem_cgroup_iter(NULL, zswap_next_=
shrink, NULL);
> > -               memcg =3D zswap_next_shrink;
> > +               next_memcg =3D READ_ONCE(zswap_next_shrink);
> > +
> > +               if (memcg !=3D next_memcg) {
> > +                       /*
> > +                        * Ours was released by offlining.
> > +                        * Use the saved memcg reference.
> > +                        */
> > +                       memcg =3D next_memcg;
> > +               } else {
> > +                       /* advance cursor */
> > +                       memcg =3D mem_cgroup_iter(NULL, memcg, NULL);
> > +                       WRITE_ONCE(zswap_next_shrink, memcg);
> > +               }
>
> I suppose I'm fine with not advancing the memcg when it is already
> advanced by the memcg offlining callback.
>

For where to restart the shrinking, as Yosry pointed, my version
starts from the last memcg (=3Dretrying failed memcg or evicting once
more)
I now realize that skipping the next memcg of offlined memcg is less
likely to happen. I am reverting it to restart from the next memcg of
zswap_next_shrink.
Which one could be better?

> >
> >                 /*
> > -                * We need to retry if we have gone through a full roun=
d trip, or if we
> > -                * got an offline memcg (or else we risk undoing the ef=
fect of the
> > -                * zswap memcg offlining cleanup callback). This is not=
 catastrophic
> > -                * per se, but it will keep the now offlined memcg host=
age for a while.
> > -                *
> >                  * Note that if we got an online memcg, we will keep th=
e extra
> >                  * reference in case the original reference obtained by=
 mem_cgroup_iter
> >                  * is dropped by the zswap memcg offlining callback, en=
suring that the
> > @@ -1434,16 +1468,25 @@ 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 =3D NULL;
> > +                       /*
> > +                        * It is an offline memcg which we cannot shrin=
k
> > +                        * until its pages are reparented.
> > +                        *
> > +                        * Since we cannot determine if the offline cle=
aner has
> > +                        * been already called or not, the offline memc=
g must be
> > +                        * put back unconditonally. We cannot abort the=
 loop while
> > +                        * zswap_next_shrink has a reference of this of=
fline memcg.
> > +                        */
> >                         spin_unlock(&zswap_shrink_lock);
> > -
> > -                       if (++failures =3D=3D MAX_RECLAIM_RETRIES)
> > -                               break;
> > -
> > -                       goto resched;
> > +                       goto iternext;
>
> Hmmm yeah in the past, I set it to NULL to make sure we're not
> replacing zswap_next_shrink with an offlined memcg, after that zswap
> offlining callback for that memcg has been completed..
>
> I suppose we can just call mem_cgroup_iter(...) on that offlined
> cgroup, but I'm not 100% sure what happens when we call this function
> on a cgroup that is currently being offlined, and has gone past the
> zswap offline callback stage. So I was just playing it safe and
> restart from the top of the tree :)
>
> I think this implementation has that behavior right? We see that the
> memcg is offlined, so we drop the lock and go to the beginning of the
> loop. We reacquire the lock, and might see that zswap_next_shrink =3D=3D
> memcg, so we call mem_cgroup_iter(...) on it. Is this safe?
>
> Note that zswap_shrink_lock only orders serializes this memcg
> selection loop with memcg offlining after it - there's no guarantee
> what's the behavior is for memcg offlining before it (well other than
> one reference that we manage to acquire thanks to
> mem_cgroup_iter(...), so that memcg has not been freed, but not sure
> what we can guarantee regarding its place in the memcg hierarchy
> tree?).

The locking mechanism in shrink_worker does not rely on what the next
memcg is.sorting stability of mem_cgroup_iter does not matter
here.
The expectation for the iterator is that it will walk through all live
memcgs. I believe mem_cgroup_iter uses parent-to-leaf ordering of
cgroup and it ensures all live cgroups are walked at least once,
regardless of its onlineness.
https://elixir.bootlin.com/linux/v6.10-rc2/source/mm/memcontrol.c#L1368

Regarding reference leak, I overlooked a scenario where a leak might
occur in the existing cleaner. although it should be rare.

When the cleaner is called on a memcg in zswap_next_shrink, the next
memcg from mem_cgroup_iter() can be an offline already-cleaned memcg,
resulting in a reference leak of the next memcg from the cleaner. We
should implement the same online check in the cleaner, like this:


```c
void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg)
{
        struct mem_cgroup *next;

        /* lock out zswap shrinker walking memcg tree */
        spin_lock(&zswap_shrink_lock);
        if (zswap_next_shrink =3D=3D memcg) {
                next =3D zswap_next_shrink;
                do {
                        next =3D mem_cgroup_iter(NULL, next, NULL);
                        WRITE_ONCE(zswap_next_shrink, next);

                        spin_unlock(&zswap_shrink_lock);
                        /* zswap_next_shrink might be updated here */
                        spin_lock(&zswap_shrink_lock);

                        next =3D READ_ONCE(zswap_next_shrink);
                        if (!next)
                                break;
                } while (!mem_cgroup_online(next));
                /*
                 * We verified the next memcg is online under lock.
                 * Even if the next memcg is being offlined here, another
                 * cleaner for the next memcg is waiting for our unlock jus=
t
                 * behind us.  We can leave the next memcg reference.
                 */
        }
        spin_unlock(&zswap_shrink_lock);
}
```

As same as in shrink_worker, we must check if the next memcg is online
under the lock before leaving the ref in zswap_next_shrink.
Otherwise, zswap_next_shrink might hold the ref of offlined and cleaned mem=
cg.

Or if you are concerning about temporary storing unchecked or offlined
memcg in zswap_next_shrink, it is safe because:

1. If there is no other cleaner running for zswap_next_shrink, the ref
saved in zswap_next_shrink ensures liveness of the memcg when
reacquired.
2. Another cleaner thread may put back and replace zswap_next_shrink
with its next. We will check onlineness of the new zswap_next_shrink
under reacquired lock.
3. Even if the verified-online memcg is being offlined concurrently,
another cleaner thread must wait for our unlock. We can leave the
online memcg and rely on its respective cleaner.