RFR: JDK-8220671: Initialization race for non-JavaThread PtrQueues

[Per Liden]

Hi Kim,

On 4/1/19 9:21 AM, Kim Barrett wrote:
>> On Mar 27, 2019, at 4:27 PM, Roman Kennke <rkennke at redhat.com> wrote:
>>
>>>>> The one new use of the mutex being added by this
>>>>> change doesn't introduce such a problem, but still... I think it's
>>>>> easy to avoid that problem by adding a new mutex just to protect the
>>>>> synchronize call.
>>>>
>>>> Uh, no? Wouldn't that be missing the point above? We can't just 'avoid' the problem by protecting something by two mutexes that ought to be under one mutex.
>>>>
>>>> Or what am I missing?
>>> Moving that synchronization outside the NJTList_lock solves that deadlock
>>> problem. But the synchronization itself still needs protection; this
>>> synchronizer only supports one synchronization at a time (one of its
>>> disadvantages compared to GlobalCounter, and the reason for "SingleWriter"
>>> in the name). So we need a new lock, specifically for protecting those
>>> synchronization calls.
>>
>> Alright, ok then. Let's see that patch then. :-)
>>
>> Thanks, Roman
> 
> Here's the updated patch:
> 
> full: http://cr.openjdk.java.net/~kbarrett/8220671/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8220671/open.01.inc/
> 
> (The incremental patch might not be that interesting compared to the
> full patch.)
> 
> The incremental patch adds a new mutex for the NJT synchronizer's
> synchronize(). It also adjusts and/or moves around some comments. And
> it adds ZGC code to protect against the same issue. I'm not sure the
> ZGC code is in the right place (ZHeap); maybe it should be in
> ZAddressMask instead? I'll let Per comment on that.

I think we can/should skip the locking in ZGC all together, as it 
doesn't actually allow a concurrent GC thread to use it's thread local 
mask anyway, without also having such threads participate in the STS. 
Since they don't do that today, locking here is unnecessary and might 
just give the impression it's safe.

Other than that, I think the patch looks good. I don't need a new webrev 
if you just remove the ZGC locking.

cheers,
Per

> 
> Testing: mach5 tier1-5.  I couldn't think of a good way to make a real
> regression test here, because it's so timing sensitive.
>