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

[David Holmes]

Thanks Carsten.

David

On 14/10/2016 12:20 AM, Carsten Varming wrote:
> Dear David,
>
> The updated webrev looks good to me.
>
> Carsten
>
> On Wed, Oct 12, 2016 at 9:18 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Dan,
>
>     Thanks for looking at this.
>
>     On 13/10/2016 1:03 AM, Daniel D. Daugherty wrote:
>
>         On 10/11/16 11:08 PM, David Holmes wrote:
>
>             Bug: https://bugs.openjdk.java.net/browse/JDK-8166197
>             <https://bugs.openjdk.java.net/browse/JDK-8166197>
>             webrev: http://cr.openjdk.java.net/~dholmes/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).
>
>
>     Yes. As I said in email I did a quick check through but the
>     succession logic is sufficiently different that nothing was
>     obviously wrong in a similar way.
>
>
>         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);
>
>
>     Right.
>
>                 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.
>
>
>     See my reply to Carsten re the load's. I did miss one as we have
>     three "locking" paths that need to synchronize with the IUnlock code.
>
>     As for documenting ... for line 532 I can add something simple like:
>
>      532   ParkEvent * const w = _OnDeck; // raw load as we will just
>     return if non-NULL
>
>     For the other stores to _OnDeck ... CAS should be obvious. The
>     setting to NULL should also be quite clear as only the _OnDeck
>     thread sets to NULL to relinquish being _OnDeck once it has acquired
>     the mutex, which happens via CAS which has full barriers. None of
>     the plain stores are in the context of:
>
>      some_var = y; // write some shared-state
>      _OnDeck = NULL; // signal some_var has been updated
>
>             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.
>
>
>     I fixed the typo and also changed ONDECK to OnDeck so that we
>     generally refer to OnDeck in commentary unless specifically
>     referring to the _OnDeck field.
>
>                 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...
>
>
>     Right, nothing to do with _EntryList just making w the OnDeck thread.
>
>         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.
>
>
>     webrev updated in place with one comment and one new use of
>     load-acquire. Plus some cosmetic changes.
>
>     Thanks again,
>     David
>
>
>         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
>
>
>