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

[Daniel D. Daugherty]

On 8/28/15 12:24 PM, Tom Benson wrote:
> 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.

True... I'll mull on it since I'm tweaking the code again anyway...

Dan


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