Issue 734

[veronikakurth]

Hey @gavv,

the PR is not ready yet (obvious lack of tests, still some work on the code) but I wanted to double check whether this goes in the direction you envisioned. Especially regarding the very limited functionality of the FreeList. Another thing: Right now I use the default memory order coming from the Atomic templates (sequentially consistent ordering). This should work in principle, but deviates from what is suggested in the article you linked. To my understanding, using the stricter memory ordering might introduce some performance issues under heavy contention. Since a more fine grained memory order control is possible with the AtomicOps, I was wondering what you think about this.

[github-actions[bot]]

:robot: Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

[gavv]

Hi, sorry for late reply, I was ill. Will take a look in upcoming days, thanks for the patch!

[gavv]

Sorry again for delay. The patch looks great overall!

I wanted to double check whether this goes in the direction you envisioned [...] regarding the very limited functionality of the FreeList.

Yes, this is intended: lock-free structures typically support only a limited set of operations. In this case, only two: push_front() (add() in the article) and try_pop_front() (try_get() in the article).

This is perfect for us, because we want to use free list as a cache for a slab pool. Using LIFO is preferred over FIFO here, because recently deallocated objects have a higher chance to of still being in the CPU cache.

Another thing: Right now I use the default memory order coming from the Atomic templates (sequentially consistent ordering). This should work in principle, but deviates from what is suggested in the article you linked. To my understanding, using the stricter memory ordering might introduce some performance issues under heavy contention. Since a more fine grained memory order control is possible with the AtomicOps, I was wondering what you think about this.

Yes, lock-free operations usually have overhead, and using SEQ_CST everywhere only increases it.

Personally, I try to avoid inventing my own lock-free algorithms, and if I have to, I stick with SEQ_CST everywhere, as I don't feel proficient enough in this topic. Hence, core::Atomic is SEQ_CST-only, to keep its usage in the codebase simpler.

However, when we're implementing a well-known algorithm where the correct memory barriers were already carefully designed and reviewed by many eyeballs, using ACQ/REL is beneficial. So in this case, yes, it would be nice to use lower-level core::AtomicOps and replicate barriers from the article, just like we do in MpscQueue (which implements Dmitry Vyukov queue).

With this approach, implementation of containers like MpscQueue and FreeList can be more complicated, but based on a reference code and thus verifiable. And our custom code that uses core::Atomic, MpscQueue, etc, can remain simpler and much easier to verify and review.

Feedback on the code

1. FreeList can't inherit from List, as it doesn't support most List operations.

2. free_list_refs is accessed more frequently, so better to keep it the first field, like in the article. Also we could name it just refs, I think.

3. Let's rename pop_front() to try_pop_front(). The difference is that it can return NULL even if freelist is not empty, if there is contention and we're unlucky.

4. try_pop_front() should return NULL if list is empty, instead of panic, because the caller has no way to know if the list is empty other than by trying to call try_pop_front().

5. Let's also keep comments from the article, and add a link to it.

6. In FreeListImpl we also need unsafe_pop_front() that can be called only when the list is not being used by anyone. In FreeList we don't need to expose it, but we should use it in ~FreeList destructor to release ownership of all objects (like we do in in core::List).

7. nit: add_knowing_refcount_is_zero => add_knowing_refcount_is_zero_ because it's a private method. Also, constants like SHOULD_BE_ON_FREELIST could be moved to the .cpp file, I think.

[veronikakurth]

Sorry for the delay! I'm working on it.

[veronikakurth]

Hey @gavv, I think I finally addressed all of your comments. Tests are also included now. I'm looking forward to your review!

[gavv]

Thanks for update! I'll be able to check it on next week.

[gavv]

Thanks for update and sorry for long delay!

I've pushed a follow-up that fixes several bugs: 5a9433f01e6a91e531072c60e90c586f9ba34dca (mostly related to memory fences). I've described what exactly was changed in the commit message.