RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code

Wed Oct 21 02:44:26 UTC 2015

On 10/20/15, 8:15 PM, David Holmes wrote:
> Quick follow up here then I'll respond to updated webrev ...

Cool. Quick reply to your follow up...

>
> On 21/10/2015 12:51 AM, Daniel D. Daugherty wrote:
>> On 10/20/15, 1:53 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> Great find in getting to the bottom of this one!
>>
>> Thanks! And thanks for the fast review!
>>
>> Also, welcome back from vacation... hope you had a blast.
>
> Thanks - I did! :)
>
>>> On 20/10/2015 1:02 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have a fix for a long standing race between the lock-free
>>>> ObjectMonitor
>>>> verification code and the normal (locked) ObjectMonitor block 
>>>> allocation
>>>> code path. For this fix, I would like at least a Runtime team reviewer
>>>> and a Serviceability team reviewer. Thanks!
>>>>
>>>> JDK-8047212 
>>>> runtime/ParallelClassLoading/bootstrap/random/inner-complex
>>>>              assert(ObjectSynchronizer::verify_objmon_isinpool(inf))
>>>> failed:
>>>>              monitor is invalid
>>>> https://bugs.openjdk.java.net/browse/JDK-8047212
>>>>
>>>> Webrev URL:
>>>> http://cr.openjdk.java.net/~dcubed/8047212-webrev/0-jdk9-hs-rt/
>>>
>>>  src/share/vm/runtime/synchronizer.cpp
>>>
>>> I would have just used OrderAccess::storeStore() as per my comment in
>>> the CR.
>>
>> Saw your comment, but I already had the current fix in my stress
>> testing cycle so I went with it...
>>
>> I could use OrderAccess::storestore(), but I think I'm more fond
>> of "OrderAccess::release_store_ptr(&gBlockList, temp)". To me the
>> semantics are more clear:
>>
>>    release previous stores before storing a new value in this variable
>>
>> I _think_ OrderAccess::release_store_ptr() is also a little less
>> heavyweight than OrderAccess::storestore(), but I'd have to re-read
>> everything in orderAccess.?pp to nail that down for sure.
>>
>>
>>> But as Carsten says a "release" should be paired with an "acquire".
>>> Which suggests that in the other code that reads these variables we
>>> also need either the load_acquire() or a loadLoad() (if using
>>> storeStore()).**
>>
>> Yes, Carsten is right and I was modeling after other ObjectMonitor
>> code that doesn't do this quite right. I'll fix this to use
>> load_acquire().
>>
>>
>>>
>>> ** This symmetry is largely missing in our lock-free code, and I think
>>> we've been relying on "volatile" to act as a compiler barrier. :(
>>
>> Hey it seems to work! (famous last words)
>
> Well we're gradually getting around to cleaning it up. :)

One bug at a time... :-)

>
>>
>>> src/share/vm/runtime/vmStructs.cpp
>>>
>>> Can you not just define volatile_static_field?
>>
>> Yes, I went that way originally and then I changed my mind to
>> avoid colliding with the other format. See below.
>>
>>
>>> Why does the ptr aspect need to come into it? Also "static pointer
>>> volatile field" sounds really odd, it is a static, volatile field that
>>> happens to be a pointer-type.
>>
>> It's meant to be odd because it follows the actual decl:
>>
>>      static ObjectMonitor * volatile gBlockList;
>>
>> So "static pointer volatile field" is exactly what I have:
>>
>>      static ObjectMonitor * volatile gBlockList;
>>
>>   => (static ObjectMonitor *) volatile gBlockList;
>>
>> i.e. I have a static ObjectMonitor pointer that is volatile
>>
>>
>> Compared to these two forms:
>>
>>      static volatile ObjectMonitor * gBlockList;
>>      static ObjectMonitor volatile * gBlockList;
>>
>>   => static (volatile ObjectMonitor) * gBlockList;
>>   => static (ObjectMonitor volatile) * gBlockList;
>>
>> i.e. I have a static pointer to a volatile ObjectMonitor.
>>
>> Hopefully, this makes my reasons a bit more clear...
>
> Not really :) Yes there is a difference between a "volatile pointer to 
> Foo" and "pointer to a volatile Foo", but for the sake of vmstructs we 
> don't really seem to need to care about that. The two questions are:
> - is the field/variable static
> - is the field/variable volatile

I'll have to politely disagree:

Here's the existing volatile non-static macro:

2743 // This macro checks the type of a volatile VMStructEntry by 
comparing pointer types
2744 #define CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY(typeName, 
fieldName, type)        \
2745  {typedef type dummyvtype; typeName *dummyObj = NULL; volatile 
dummyvtype* dummy = &dummyObj->fieldName; }

And here's the new static pointer volatile macro:

2751 // This macro checks the type of a static pointer volatile 
VMStructEntry by comparing pointer types,
2752 // e.g.: "static ObjectMonitor * volatile gBlockList;"
2753 #define CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, 
fieldName, type)       \
2754  {type volatile * dummy = &typeName::fieldName; }

Yes, the variable assignments are different because we have static
versus a non-static situation, but what's more important is where
the "volatile" is positioned.

In the existing volatile non-static macro, the volatile piece is:

     volatile dummyvtype* dummy = &dummyObj->fieldName;

and in the new static pointer volatile macro, the volatile piece is:

     type volatile * dummy = &typeName::fieldName;

So the CHECK_VOLATILE_NONSTATIC_XXX macro has the "volatile" before
the data type... and the CHECK_STATIC_PTR_VOLATILE_XXX macro
has the "volatile" after the data type...

Dan

>
> Cheers,
> David
>
>> Dan
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Testing: Aurora Adhoc RT-SVC nightly batch
>>>>           4 inner-complex fastdebug parallel runs for 4+ days and
>>>>             600K iterations without seeing this failure; the 
>>>> experiment
>>>>             is still running; final results to be reported at the end
>>>>             of the review cycle
>>>>           JPRT -testset hotspot
>>>>
>>>> This fix:
>>>>
>>>> - makes ObjectMonitor::gBlockList volatile
>>>> - uses "OrderAccess::release_store_ptr(&gBlockList, temp)" to
>>>>    make sure the new block updates _happen before_ gBlockList is
>>>>    changed to refer to the new block
>>>> - add SA support for a "static pointer volatile" field like:
>>>>
>>>>      static ObjectMonitor * volatile gBlockList;
>>>>
>>>> See the following link for a nice description of what "volatile"
>>>> means in the different positions on a variable/parameter decl line:
>>>>
>>>> http://www.embedded.com/electronics-blogs/beginner-s-corner/4023801/Introduction-to-the-Volatile-Keyword 
>>>>
>>>>
>>>>
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>>