Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231024203302.1920362-3-nphamcs@gmail.com> <20231101012614.186996-1-nphamcs@gmail.com>
 <CAJD7tkZV9NaPUzjZYQAx=D-vDEXZH46=Dn5zEuMHnsybT+HLAw@mail.gmail.com>
In-Reply-To: <CAJD7tkZV9NaPUzjZYQAx=D-vDEXZH46=Dn5zEuMHnsybT+HLAw@mail.gmail.com>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Tue, 31 Oct 2023 18:41:33 -0700
Message-ID: <CAKEwX=N3MsBJ2YW7N=am4MSAqZC5O32Ooz96ec_J_RyhCts=qg@mail.gmail.com>
Subject: Re: [PATCH v4 2/5] zswap: make shrinking memcg-aware
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org,
        cerasuolodomenico@gmail.com, sjenning@redhat.com,
        ddstreet@ieee.org, vitaly.wool@konsulko.com, mhocko@kernel.org,
        roman.gushchin@linux.dev, shakeelb@google.com,
        muchun.song@linux.dev, chrisl@kernel.org, linux-mm@kvack.org,
        kernel-team@meta.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 31, 2023 at 6:33=E2=80=AFPM Yosry Ahmed <yosryahmed@google.com>=
 wrote:
>
> On Tue, Oct 31, 2023 at 6:26=E2=80=AFPM Nhat Pham <nphamcs@gmail.com> wro=
te:
> >
> > cc-ing Johannes, Roman, Shakeel, Muchun since you all know much more
> > about memory controller + list_lru reparenting logic than me.
> >
> > There seems to be a race between memcg offlining and zswap=E2=80=99s
> > cgroup-aware LRU implementation:
> >
> > CPU0                            CPU1
> > zswap_lru_add()                 mem_cgroup_css_offline()
> >     get_mem_cgroup_from_objcg()
> >                                     memcg_offline_kmem()
> >                                         memcg_reparent_objcgs()
> >                                         memcg_reparent_list_lrus()
> >                                             memcg_reparent_list_lru()
> >                                                 memcg_reparent_list_lru=
_node()
> >     list_lru_add()
> >                                                 memcg_list_lru_free()
> >
> >
> > Essentially: on CPU0, zswap gets the memcg from the entry's objcg
> > (before the objcgs are reparented). Then it performs list_lru_add()
> > after the list_lru entries reparenting (memcg_reparent_list_lru_node())
> > step. If the list_lru of the memcg being offlined has not been freed
> > (i.e before the memcg_list_lru_free() call), then the list_lru_add()
> > call would succeed - but the list will be freed soon after. The new
> > zswap entry as a result will not be subjected to future reclaim
> > attempt. IOW, this list_lru_add() call is effectively swallowed. And
> > worse, there might be a crash when we invalidate the zswap_entry in the
> > future (which will perform a list_lru removal).
> >
> > Within get_mem_cgroup_from_objcg(), none of the following seem
> > sufficient to prevent this race:
> >
> >     1. Perform the objcg-to-memcg lookup inside a rcu_read_lock()
> >     section.
> >     2. Checking if the memcg is freed yet (with css_tryget()) (what
> >     we're currently doing in this patch series).
> >     3. Checking if the memcg is still online (with css_tryget_online())
> >     The memcg can still be offlined down the line.
> >
> >
> > I've discussed this privately with Johannes, and it seems like the
> > cleanest solution here is to move the reparenting logic down to release
> > stage. That way, when get_mem_cgroup_from_objcg() returns,
> > zswap_lru_add() is given an memcg that is reparenting-safe (until we
> > drop the obtained reference).
>
> The objcgs hold refs to the memcg, which are dropped during
> reparenting. How can we do reparenting in the release stage, which
> IIUC happens after all refs are dropped?

Oh I meant just the list_lru reparenting. The list_lru themselves
don't hold any ref I believe, right? Then it's safe to perform this at
the release stage.

(also, I think I might have messed up the encoding for the email
above. Let me know if people cannot view it, and I'll resend it :( )