[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8166197: assert(RelaxAssert || w != Thread::current()->_MutexEvent) failed: invariant
From:       David Holmes <david.holmes () oracle ! com>
Date:       2016-10-13 23:13:32
Message-ID: f5c777cf-416a-b53c-7bf0-47e193d20861 () oracle ! com
[Download RAW message or body]

Thanks Dan!

David

On 14/10/2016 2:24 AM, Daniel D. Daugherty wrote:
>> webrev updated in place with one comment and one new use of load-acquire.
>> Plus some cosmetic changes.
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8166197/webrev/
>
> src/share/vm/runtime/mutex.cpp
>     No comments.
>
> Thumbs up!
>
> Dan
>
>
>
> On 10/12/16 7:18 PM, David Holmes 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
>>>> 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
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]