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

[peterz@infradead.org]

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().