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

peterz
2020-08-28T15:30:15Z

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