lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJD7tkZMTJKYR+au2rjP1v+c8PvdP4D39j86tHg=o2riKGYynQ@mail.gmail.com>
Date: Fri, 18 Oct 2024 13:40:18 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Kairui Song <ryncsn@...il.com>
Cc: Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Johannes Weiner <hannes@...xchg.org>, 
	Nhat Pham <nphamcs@...il.com>, Chengming Zhou <chengming.zhou@...ux.dev>, 
	Chris Li <chrisl@...nel.org>, Barry Song <v-songbaohua@...o.com>, 
	"Huang, Ying" <ying.huang@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm, zswap: don't touch the XArray lock if there is no
 entry to free

On Fri, Oct 18, 2024 at 1:01 PM Kairui Song <ryncsn@...il.com> wrote:
>
> On Sat, Oct 19, 2024 at 3:46 AM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > On Sat, Oct 19, 2024 at 03:25:25AM +0800, Kairui Song wrote:
> > >       if (xa_empty(tree))
> > >               return;
> > >
> > > -     entry = xa_erase(tree, offset);
> > > -     if (entry)
> > > +     rcu_read_lock();
> > > +     entry = xas_load(&xas);
> > > +     if (entry) {
> >
> > You should call xas_reset() here.

Oh I thought xas_reload() is enough here to check that the entry is
still there after the lock is acquired. Do we have to start the walk
over after holding the lock?

If yes, it seems like that would be equivalent to the following:

entry = xa_load(tree, offset);
if (entry)
           xa_erase(tree, offset);

>> And I'm not sure it's a great idea to
> > spin waiting for the xa lock while holding the RCU read lock?  Probably
> > not awful but I could easily be wrong.

If we end up using xa_load() and xa_erase() then we avoid that, but
then we'd need to walk the xarray twice. I thought we could avoid the
rewalk with xas_reload(). I am not sure if the xa_load() check would
still be worth it at this point -- or maybe the second walk will be
much faster as everything will be cache hot? Idk.

Matthew, any prior experience with such patterns of lockless lookups
followed by a conditional locked operation?

>
> Thanks for the review. I thought about it, that could cancel this optimization.
>
> Oh, and there is a thing I forgot to mention (maybe I should add some
> comments about it?). If xas_load found an entry, that entry must be
> pinned by HAS_CACHE or swap slot count right now, and one entry can
> only be freed once.
> So it should be safe here?
>
> This might be a little fragile though, maybe this optimization can
> better be done after some zswap invalidation path cleanup.

The only guarantee that we are requiring from the caller here is that
the swap entry is stable, i.e. is not freed and reused while
zswap_invalidate() is running. This seems to be a reasonable
assumption, or did I miss something here?

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ