RFR: 8263551: Provide shared lock-free FIFO queue implementation
Man Cao
manc at openjdk.java.net
Mon Mar 15 19:53:09 UTC 2021
On Sun, 14 Mar 2021 00:53:03 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> src/hotspot/share/utilities/lockFreeQueue.hpp line 99:
>>
>>> 97: } else {
>>> 98: assert(get_next(*old_tail) == NULL, "invariant");
>>> 99: Atomic::store(next_ptr(*old_tail), &first);
>>
>> I changed this store from a normal store to an Atomic store. Otherwise there is a data race between this store and the load of _head->_next in pop().
>
> As David said, Atomic::store doesn't indicate any ordering; it's a relaxed atomic store. The old code was `old_tail->set_next(&first)`, which was hard-wired to the element type being BufferNode (which was okay in its place). But the BufferNode code predates consistent use of Atomic when accessing atomic/volatile data, so don't currently use Atomic::load/store. In this generic hoist we no longer want to assume next and set_next functions, just the next_ptr function that returns a pointer to atomic/volatile. I see that you added get_next, which uses Atomic::load; I think there should be an associated set_next, as there are other places that need it.
Thanks for the reviews and feedbacks. I will address the other comments soon.
For this issue, agreed that I misused the term "data race". It is an inconsistent use of Atomic APIs but not really a data race. I will use set_next() here.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2986
More information about the hotspot-dev
mailing list