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