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

[David Holmes]

Hi Dan,

Great find in getting to the bottom of this one!

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. 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()).**

** 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. :(

src/share/vm/runtime/vmStructs.cpp

Can you not just define volatile_static_field? 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.

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
>