Received-SPF: pass (google.com: domain of linux-kernel+bounces-39273-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: <20240120024007.2850671-1-yosryahmed@google.com>
 <20240120024007.2850671-3-yosryahmed@google.com> <20240122201906.GA1567330@cmpxchg.org>
 <CAJD7tkaATS48HVuBfbOmPM3EvRUoPFr66WhF64UC4FkyVH5exg@mail.gmail.com>
 <20240123153851.GA1745986@cmpxchg.org> <CAJD7tkasHsRnT_75-TXsEe58V9_OW6m3g6CF7Kmsvz8CKRG_EA@mail.gmail.com>
 <20240123201234.GC1745986@cmpxchg.org> <CAJD7tkZC6w2EaE=j2NEVWn1s7Lo2A7YZh8LiZ+w72jQzFFWLUQ@mail.gmail.com>
 <f3fa799f-1815-4cfe-abc8-3ba929fcd1ba@bytedance.com> <CAF8kJuOv0FHsHgp=i7ZzjTpvZ7fN8uOQ3DT=8Q2eYWzRM7Hsgw@mail.gmail.com>
 <CAJD7tkY=qy+dfKacFOBx4uW6hDJwf20ODBgVWRP919hEY5URnQ@mail.gmail.com> <CAF8kJuOvjJ1ARzAGMVheDgq6tpUM76BZ9GggWj7CB=J3XgU6mw@mail.gmail.com>
In-Reply-To: <CAF8kJuOvjJ1ARzAGMVheDgq6tpUM76BZ9GggWj7CB=J3XgU6mw@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 25 Jan 2024 13:01:09 -0800
Message-ID: <CAJD7tkZV0+aPD0_pcFONnsGXC0__62yyQQmXX8jj5G=-QAySsg@mail.gmail.com>
Subject: Re: [PATCH 2/2] mm: zswap: remove unnecessary tree cleanups in zswap_swapoff()
To: Chris Li <chrisl@kernel.org>
Cc: Chengming Zhou <zhouchengming@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, 
	Andrew Morton <akpm@linux-foundation.org>, Nhat Pham <nphamcs@gmail.com>, 
	Huang Ying <ying.huang@intel.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 25, 2024 at 11:04=E2=80=AFAM Chris Li <chrisl@kernel.org> wrote=
:
>
> On Thu, Jan 25, 2024 at 12:02=E2=80=AFAM Yosry Ahmed <yosryahmed@google.c=
om> wrote:
> >
> > > > // lru list lock held
> > > > shrink_memcg_cb()
> > > >   swpentry =3D entry->swpentry
> > > >   // Don't isolate entry from lru list here, just use list_lru_putb=
ack()
> > > >   spin_unlock(lru list lock)
> > > >
> > > >   folio =3D __read_swap_cache_async(swpentry)
> > > >   if (!folio)
> > > >     return
> > > >
> > > >   if (!folio_was_allocated)
> > > >     folio_put(folio)
> > > >     return
> > > >
> > > >   // folio is locked, swapcache is secured against swapoff
> > > >   tree =3D get tree from swpentry
> > > >   spin_lock(&tree->lock)
> > >
> > > That will not work well with zswap to xarray change. We want to remov=
e
> > > the tree lock and only use the xarray lock.
> > > The lookup should just hold xarray RCU read lock and return the entry
> > > with ref count increased.
> >
> > In this path, we also invalidate the zswap entry, which would require
> > holding the xarray lock anyway.
>
> It will drop the RCU read lock after finding the entry and re-acquire
> the xarray spin lock on invalidation. In between there is a brief
> moment without locks.

If my understanding is correct, at that point in the code writeback is
guaranteed to succeed unless the entry had already been removed from
the tree. So we can use xa_cmpxchg() as I described earlier to find
and remove the entry in the tree only if it exists. If it does, we
continue with the writeback, otherwise we abort. No need for separate
load and invalidation. zswap_invalidate_entry() can return a boolean
(whether the entry was found and removed from the tree or not).