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

[David Holmes]

Quick follow up here then I'll respond to updated webrev ...

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. :)

>
>> 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

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
>>>