RFR (S) 8049304: race between VM_Exit and _sync_FutileWakeups->inc()

[Tom Benson]

Hi Dan,


On 8/28/2015 2:12 PM, Daniel D. Daugherty wrote:
>  . . .
>>>
>>> Not easily in ReenterI(), the OM_PERFDATA_OP(FutileWakeups, inc()) call
>>> is at the bottom of the for-loop and the OrderAccess::fence() call at
>>> the end of the function is outside the loop. This would result in lost
>>> FutileWakeups increments.
>> Yes, you'd have to keep a local count, and then add the total outside 
>> the loop for both Enter/Reenter, after the fence.  But I see what you 
>> mean about the other exit paths in Enter.   (The more I look at this 
>> code, the more I remember it...
>
> I'm sure that is a wonderfully loving memory too!

Absolutely!  8^)

>
>
>> BTW, are those knob_ setting defaults ever going to be moved to a 
>> platform specific-module?   That was my beef (well, one of them) in 2 
>> different ports.  Or is the goal to make monitors so well self-tuning 
>> that they can go away? Sorry for the digression... 8^))
>
> Dice is working on another idea to move tuning to a separate loadable
> module which is why we deferred the "adaptive spin" and "SpinPause on
> SPARC" buckets for the Contended Locking project.
>
>
>> At any rate, as you say, perhaps it's not worth it to leverage the 
>> fences, though it could be done.
>
> OK so we're agreed on no change here.
>
>
>>
>>>
>>> So in ReenterI() the OM_PERFDATA_OP(FutileWakeups, inc()) call 
>>> immediately
>>> follows an OrderAccess::fence() call. Doesn't that make that 
>>> increment as
>>> "safe" as it can be without having a real lock?
>>>
>>>
>>>> You've got a release() (and and short nap!) with the store in 
>>>> PerfDataManager::destroy() to try to close the window somewhat.
>>>
>>> Yes, I modeled that after:
>>>
>>> src/share/vm/runtime/perfMemory.cpp:
>>>
>>>     83  void PerfMemory::initialize() {
>>> <snip>
>>>    156    OrderAccess::release_store(&_initialized, 1);
>>>    157  }
>>>
>>>
>>>> But I think rather than the release_store()  you used, you want a 
>>>> store, followed by a release().   release_store() puts a fence 
>>>> before the store to ensure earlier updates are seen before the 
>>>> current one, no?
>>>
>>> Yup, and I see I got my reasoning wrong. The code I modeled
>>> is right because you want to flush all the inits and it's OK
>>> if the _initialized transition from '0' -> '1' is lazily seen.
>>>
>>> For my shutdown use, we are transitioning from '1' -> '0' and
>>> we need that to be seen proactively so:
>>>
>>>     OrderAccess::release_store(&_has_PerfData, 0);
>>>     OrderAccess::storeload();
>>>
>>> which is modeled after _owner field transitions from non-zero
>>> -> NULL in ObjectMonitor.cpp
>>>
>>
>> It's not clear to me why the store needs to be a release_store in 
>> this case, as long as the storeload() follows it.  You're not 
>> protecting any earlier writes. ?
>
> I'm following the model that Dice uses for ObjectMonitors when we
> change the _owner field from non-NULL -> NULL. There are some long
> comments in the ObjectMonitor.cpp stuff that talk about why it is
> done this way. I'm planning to file a cleanup bug for the non-NULL
> -> NULL transition stuff because the comments and code are not
> consistent.
>

But that's in lock exit, so the release is needed to ensure all 
outstanding writes are seen before the owner is set to null. There's 
nothing analogous in this case.

However, since this will be executed once per JVM shutdown, having an 
extra release() isn't a big deal...
Tom