RFR: 8256474: Migrate Mutex _owner accesses to use Atomic operations

[David Holmes]

Hi Kim,

On 24/11/2020 12:16 pm, Kim Barrett wrote:
> On Mon, 23 Nov 2020 23:08:28 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>> Simple update to move away from volatile fields and use Atomic::load/store on racy accesses.
>>
>> Thanks,
>> David
> 
> Changes requested by kbarrett (Reviewer).
> 
> src/hotspot/share/runtime/mutex.hpp line 89:
> 
>> 87:   // the low-level _lock, or to NULL after it has released the _lock. Accesses by any thread other
>> 88:   // than the lock owner are inherently racy.
>> 89:   Thread* _owner;
> 
> My understanding of the idiom of using Atomic always rather than relying on any semantics for volatile is that the volatile qualifier is left on the variable, as a marker/indicator/clue that it is interesting.  This will also help us find such if/when we adopt some kind of atomic class for encapsulation (whether std::atomic or something like what was proposed for JDK-8247213) and help track adoption progress.  So I think the volatile qualifier should be retained.

That was not my understanding. See for example Dan's very recent fix in 
this area for "8238174: migrate ObjectMonitor::_owner field away from 
C++ volatile semantics". But after checking some recent code from the 
people I thought favoured the "no volatile" use, I still see volatile 
being used. So I'm somewhat confused as to what we think we're supposed 
to be doing here. The description in:

https://bugs.openjdk.java.net/browse/JDK-8234192

does not make it clear how we are to proceed, but my comment from a year 
ago did summarise what I thought was the current thinking:

"As an interim step there are moves afoot to transition away from using 
volatile variables and plain loads/stores that we expect/require to be 
atomic (not torn) accesses (for aligned 32-bit and 64-bit variables at 
least), and to use our own Atomic::load and Atomic::store (which 
themselves rely on the same undefined semantics of volatile but at least 
it is more localised)".

This was, to the best of my recollection, based on comments from Stefan 
K., Erik O, and Robbin E.. But now I see recent code from Erik that 
doesn't do this, but keeps the volatile.

I don't really care one way or the other but we need to all be on the 
same page here.

> src/hotspot/share/runtime/mutex.hpp line 87:
> 
>> 85:  private:
>> 86:   // The _owner field is only set by the current thread, either to itself after it has acquired
>> 87:   // the low-level _lock, or to NULL after it has released the _lock. Accesses by any thread other
> 
> s/after/before/

Good catch! Fixed.

Thanks,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/1402
>