linux-kernel - Re: [RFC][PATCH 6/7] freelist: Lock less freelist

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]

Date:   Fri, 28 Aug 2020 17:29:46 +0200
From:   peterz@...radead.org
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, mhiramat@...nel.org,
        Eddy_Wu@...ndmicro.com, x86@...nel.org, davem@...emloft.net,
        rostedt@...dmis.org, naveen.n.rao@...ux.ibm.com,
        anil.s.keshavamurthy@...el.com, linux-arch@...r.kernel.org,
        cameron@...dycamel.com, will@...nel.org, paulmck@...nel.org
Subject: Re: [RFC][PATCH 6/7] freelist: Lock less freelist

On Fri, Aug 28, 2020 at 04:46:52PM +0200, Oleg Nesterov wrote:
> On 08/27, Peter Zijlstra wrote:
> >
> >  1 file changed, 129 insertions(+)
> 
> 129 lines! And I spent more than 2 hours trying to understand these
> 129 lines ;) looks correct...

Yes, even though it already has a bunch of comments, I do feel we can
maybe improve on that a little.

For now I went for a 1:1 transliteration of the blog post though.

> > +			/*
> > +			 * Yay, got the node. This means it was on the list,
> > +			 * which means should-be-on-freelist must be false no
> > +			 * matter the refcount (because nobody else knows it's
> > +			 * been taken off yet, it can't have been put back on).
> > +			 */
> > +			WARN_ON_ONCE(atomic_read(&head->refs) & REFS_ON_FREELIST);
> > +
> > +			/*
> > +			 * Decrease refcount twice, once for our ref, and once
> > +			 * for the list's ref.
> > +			 */
> > +			atomic_fetch_add(-2, &head->refs);
> 
> Do we the barriers implied by _fetch_? Why can't atomic_sub(2, refs) work?

I think we can, the original has std::memory_order_relaxed here. So I
should've used atomic_fetch_add_relaxed() but since we don't use the
return value, atomic_sub() would work just fine too.

> > +		/*
> > +		 * OK, the head must have changed on us, but we still need to decrement
> > +		 * the refcount we increased.
> > +		 */
> > +		refs = atomic_fetch_add(-1, &prev->refs);
> 
> Cosmetic, but why not atomic_fetch_dec() ?

The original had that, I didn't want to risk more bugs by 'improving'
things. But yes, that can definitely become dec().