P4
The following text is written by [~eosterlund] and comes from this review comment: https://github.com/openjdk/jdk/pull/19454#discussion_r1771021064
What strikes me a bit is that the EntryList vs cxq loads have inconsistent ordering in the runtime and the different ports. Notably, the x86 port loads EntryList first while here for example we load cxq first. This adds some cognitive overhead reasoning about the implications, at least for me. In particular, with respect to the ordering we see here where EntryList is loaded first, the following race is possible (which is not possible in the x86 intrinsic):
1. Thread 1 enters the monitor.
2. Thread 2 tries to enter the monitor, fails, adds itself to cxq, tries again, and eventually parks.
3. Thread 1 starts exiting the monitor and runs all the instructions up to and including the EntryList load above on line 221, which yields an empty EntryList.
4. Thread 3 enters the monitor since it is no longer owned.
5. Thread 3 exits the monitor, and moves cxq to EntryList in the process while still holding the lock.
6. Thread 1 runs the next instruction loading cxq on line 222, resulting in an empty list.
7. Thread 1 draws the conclusion that there is no thread waiting for the monitor, even though Thread 2 has waited for the monitor since before Thread 2 released it.
Even though this choice of ordering between reading EntryList and cxq allows a waiter to be completely unnoticed throughout the monitorexit procedure due to unfortunate interleavings, which is weird, the protocol deals with this just fine. Since Thread 2 managed to enter the monitor, the responsibility of ensuring liveness of the monitor becomes the responsibility of Thread 2 which will make Thread 1 the successor and unpark Thread 1.
I'm not proposing we have the "other" more intuitive ordering of loading cxq first which removes this race so we don't have to think about it. Enforcing that ordering requires an extra loadload fence, which isn't free. I think what I would prefer to see, is that we at least use a consistent ordering so we don't have to think about why some platforms use one ordering while others use another one and what different races only affect some platforms. And I'd prefer if this less obvious ordering of reading EntryList before cxq is the championed ordering, with a comment in the shared code explaining why it's okay that waiters slip undetected between the two loads. Because without explicit fencing, that's an order that has to be valid anyway, unless we jam in some loadload fences.
What strikes me a bit is that the EntryList vs cxq loads have inconsistent ordering in the runtime and the different ports. Notably, the x86 port loads EntryList first while here for example we load cxq first. This adds some cognitive overhead reasoning about the implications, at least for me. In particular, with respect to the ordering we see here where EntryList is loaded first, the following race is possible (which is not possible in the x86 intrinsic):
1. Thread 1 enters the monitor.
2. Thread 2 tries to enter the monitor, fails, adds itself to cxq, tries again, and eventually parks.
3. Thread 1 starts exiting the monitor and runs all the instructions up to and including the EntryList load above on line 221, which yields an empty EntryList.
4. Thread 3 enters the monitor since it is no longer owned.
5. Thread 3 exits the monitor, and moves cxq to EntryList in the process while still holding the lock.
6. Thread 1 runs the next instruction loading cxq on line 222, resulting in an empty list.
7. Thread 1 draws the conclusion that there is no thread waiting for the monitor, even though Thread 2 has waited for the monitor since before Thread 2 released it.
Even though this choice of ordering between reading EntryList and cxq allows a waiter to be completely unnoticed throughout the monitorexit procedure due to unfortunate interleavings, which is weird, the protocol deals with this just fine. Since Thread 2 managed to enter the monitor, the responsibility of ensuring liveness of the monitor becomes the responsibility of Thread 2 which will make Thread 1 the successor and unpark Thread 1.
I'm not proposing we have the "other" more intuitive ordering of loading cxq first which removes this race so we don't have to think about it. Enforcing that ordering requires an extra loadload fence, which isn't free. I think what I would prefer to see, is that we at least use a consistent ordering so we don't have to think about why some platforms use one ordering while others use another one and what different races only affect some platforms. And I'd prefer if this less obvious ordering of reading EntryList before cxq is the championed ordering, with a comment in the shared code explaining why it's okay that waiters slip undetected between the two loads. Because without explicit fencing, that's an order that has to be valid anyway, unless we jam in some loadload fences.
- links to
-
Commit(master)
openjdk/jdk/eb3669a5
-
Review(master)
openjdk/jdk/21584