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

[Daniel D. Daugherty]

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.


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


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

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