RFR: 8166197: assert(RelaxAssert || w != Thread::current()->_MutexEvent) failed: invariant

[Daniel D. Daugherty]

On 10/11/16 11:08 PM, David Holmes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166197
> webrev: http://cr.openjdk.java.net/~dholmes/8166197/webrev/

Very nice catch! We should check the ObjectMonitor succession code for
similar issues (my task).


src/share/vm/runtime/mutex.cpp
     L466:   if ((NativeMonitorFlags & 32) && CASPTR (&_OnDeck, NULL, 
UNS(ESelf)) == 0) {
         Thanks for fixing this bug also!

     L477:   while (OrderAccess::load_ptr_acquire(&_OnDeck) != ESelf) {
         So you've changed this load of _OnDeck to use load-acquire
         which matches the new store-release on L595:

         OrderAccess::release_store_ptr(&_OnDeck, w);

         What about the other loads of _OnDeck or stores to _OnDeck?
         There should at least be a new comment explaining why we don't
         need an OrderAccess operation for those. Update: I see you
         changed one other load of _OnDeck on L1061. Now I'm really
         wanting comments for the other _OnDeck loads and stores. :-)

         Update: I see Carsten V. asked about this in a slightly different
         way.

     L590:     // Pass onDeck to w, ensuring that _EntryList has been 
set first.
         Typo: 'onDeck' -> 'OnDeck'

         I suspect you don't want to fix all this CamelCase usage to meet
         HotSpot style. I did that for most of the ObjectMonitor code and
         it was painful. We could clean it up early in JDK10.

         Update: I see Carsten has a comment about this comment also. I
         don't think I quite agree that we're "passing" _EntryList to w,
         but I can be convinced otherwise...

Again, very nice catch! I'd like to see another webrev with the other
_OnDeck loads and stores either updated for OrderAccess ops or some
comment explaining why it's not needed.

Dan


>
> In IUnlock we have the following succession code to wakeup the 
> "onDeck" thread:
>
>  ParkEvent * List = _EntryList;
>   if (List != NULL) {
>     // Transfer the head of the EntryList to the OnDeck position.
>     // Once OnDeck, a thread stays OnDeck until it acquires the lock.
>     // For a given lock there is at most OnDeck thread at any one 
> instant.
>    WakeOne:
>     assert(List == _EntryList, "invariant");
>     ParkEvent * const w = List;
>     assert(RelaxAssert || w != Thread::current()->_MutexEvent, 
> "invariant");
>     _EntryList = w->ListNext;
>     // as a diagnostic measure consider setting w->_ListNext = BAD
>     assert(UNS(_OnDeck) == _LBIT, "invariant");
>     _OnDeck = w;  // pass OnDeck to w.
>
> It is critical that the update to _EntryList happens before we set 
> _OnDeck, as as soon as _OnDeck is set the selected thread (which need 
> not yet have parked) can acquire the mutex, complete its critical 
> section and proceed to unlock the mutex, and so execute IUnlock in 
> parallel with the original thread. If the write to _EntryList has not 
> yet happened that second thread finds itself still at the head of 
> _EntryList and so the assert fires. If the write to _EntryList happens 
> after the load "List = _EntryList", then the first assert can also fire.
>
> Preferred fix today is to use OrderAccess::release_store(&_OnDeck, w) 
> with a matching load_acquire(&_OnDeck) in the ILock code:
>
>   while (_OnDeck != ESelf) {
>     ParkCommon(ESelf, 0);
>   }
>
> and corresponding "raw" lock code. Also fixed a couple of typos.
>
> Thanks,
> David