RFR: 8256383: PlatformMutex::try_lock has different semantics on Windows and Posix

[David Holmes]

Hi Stefan,

Thanks for the review (and the initial report!).

On 17/11/2020 5:29 pm, Stefan Karlsson wrote:
> On Tue, 17 Nov 2020 05:41:44 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>> Mutex::try_lock has to allow for the possibility that the PlatformMutex::try_lock allows recursive locking.
>>
>> Added additional commentary on the semantics provided by Mutex and the PlatformMutex classes.
>>
>> Testing: GHA, mach5 tiers 1-3
> 
> Thanks for updating Mutex::try_lock and adding the comments!
> 
> src/hotspot/share/runtime/mutex.cpp line 147:
> 
>> 145:   // safepoint state, but can check blocking state.
>> 146:   check_block_state(self);
>> 147:   if (_owner == NULL && _lock.try_lock()) {
> 
> It's not obvious that's it is correct to check == NULL. Couldn't there be a lingering value from another thread, causing a spurious failure to acquire the lock? Could this be changed to _owner != self? That way we only check for a value that only the current thread could have written.
> 
> Also, this is a racy read, should this be using Atomic::load?

Yes it is a racy read but do we care? The only value of _owner that's 
guaranteed true is if it equals self (which would be a usage bug so we 
expect it to be really really really rare). Otherwise we are indeed 
racing with a thread that is releasing the Mutex (or another thread 
acquiring it!), but that race exists regardless.

If I check for != self then it forces us into the PlatformMutex code 
unnecessarily in most cases. But as you and Thomas both seem unsettled 
by this I can change it. :)

Using Atomic::load is a good idea just from a code hygiene perspective, 
but doesn't change the raciness.

Thanks,
David

> -------------
> 
> Changes requested by stefank (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/1247
>