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