boostorg / boostorg/lockfree

lockfree queue: memory orderings are questionable, 1.68.0

Open
#44 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
160
Forks
99
PR merge metrics
No merged PRs in 30d

Description

The possible problems are in queue::do_push and queue::pop.
I'm not versed in the lingo of atomics and memory orderings, so my phrasing might be a bit off - but I'm quite sure my questions are correct.

(1) double loads of tail_ and head_ in do_push and pop

First let me mention that in the original algorithm by Scott and Michael these functions have a double load of tail and head (respectively), and then the values of the first and second load are compared.
The purpose of these are to make sure the node has not been freed (std::free()) already, and then the &(node->next) might become a non dereferenceable address, or else its value could be randomly changed to false-positively pass the tag check.
However in any case that the nodes are not freed but rather put in a free-list, we don’t have this problem at all.

The reason they work in the algorithm, is because the algorithm is sequentially consistent memory:

push: the node can be freed only after tail_ has been exchanged to a value further ahead in the list than the node itself. If the value is exchanged so in any thread, it would either be visible by the second load of tail_ or it would happen after it, due to sequential consistency.

pop: basically same explanation for head_

So, our queue is using weak memory ordering, therefore the above explanations do not apply.
In case the node is actually freed, rather than put in a free-list, we need some other way to make sure &(node->next) is actually still dereferenceable.
For example, make the second loads (of tail and head) a read-modify-write operation (not for the purpose of changing the value, but for the purpose of having a strong order of visibility with the actual value exchanges).

I know that after c++11, for 14/17/20 the relationships between seq_cst and weak order operations have been strengthened. Is this what is assumed here? That the seq_cst exchanges of tail_ and head_ are strongly ordered with these second loads?

(2) questionable memory ordering in do_push()
```
tagged_node_handle tail = tail_.load(memory_order_acquire);
node * tail_node = pool.get_pointer(tail);
tagged_node_handle next = tail_node->next.load(memory_order_acquire);
node * next_ptr = pool.get_pointer(next);
tagged_node_handle tail2 = tail_.load(memory_order_acquire);
```
(2.1)
So, why would the second load of tail_ be mo_acquire?!
What is it acquiring for?! There are no further loads after that.
If at all it is needed, it could just as well be mo_relaxed.

(2.2)
Why would tail_node->next.load() be mo_acquire?!
If it is acquiring for the second tail_.load(), it could only be if some other thread had a matching:
tail_.store()
tail_node->store( …, mo_release );
But this doesn’t exist anywhere.
The only node->next mo_release in the code is later in do_push() and there are no tail_.store() before it, only an tail_.exhange() after.

(3) questionable memory ordering in pop()
```
tagged_node_handle head = head_.load(memory_order_acquire);
node * head_ptr = pool.get_pointer(head);
tagged_node_handle tail = tail_.load(memory_order_acquire);
tagged_node_handle next = head_ptr->next.load(memory_order_acquire);
node * next_ptr = pool.get_pointer(next);
tagged_node_handle head2 = head_.load(memory_order_acquire);
```
(3.1)
Why would the second head_.load be mo_aquire?!
What is it acquiring for?
If at all needed, it could be mo_relaxed

(3.2)
Why does head_ptr->next.load() has mo_acquire?!
As if it’s acquiring for the second head_.load()
But there’s no matching release for this memory.
edit: However, it needs to be at least a mo_consume for the following reading of the payload.

(4) Seems to me, in order to make sense of 2.2 and 3.2 above, maybe add
head_ptr->next.store(..., mo_release )
inside pop() after the head.compare_exchange succeeded.
Then at least it would be releasing for the head exchange and also for tail exchange (because the tail exchange happens before the head exchange, in the same pop() or earlier).

(5) Why does pop() copy the payload before the head.compare_exchange every loop, instead of only once after it succeeds? This is a performance difference.
```
detail::copy_payload(next_ptr->data, ret);
tagged_node_handle new_head(pool.get_handle(next), head.get_next_tag());
if (head_.compare_exchange_weak(head, new_head)) {
...
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading queue::do_push and queue::pop, focusing on the listed acquire loads, release operations, and compare_exchange calls in the C++ lock-free queue implementation. Trace the memory-ordering and node-lifetime assumptions raised in points (1)–(5); done means reaching a documented, validated resolution for the suspected safety and performance issues.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.