RFR: 8320318: ObjectMonitor Responsible thread [v3]

[Fredrik Bredberg]

On Mon, 23 Sep 2024 12:00:27 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 222:
>> 
>>> 220:   // Check if the entry lists are empty.
>>> 221:   ldr(rscratch1, Address(tmp, ObjectMonitor::EntryList_offset()));
>>> 222:   ldr(tmpReg, Address(tmp, ObjectMonitor::cxq_offset()));
>> 
>> 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 explainin...
>
> This should be in a follow-up RFE with this description.  Thanks for the description.

I'll create a new follow-up RFE.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1771500290