RFR(XS) for PeriodicTask_lock cleanup (8072439)

[David Holmes]

On 26/02/2015 2:46 PM, Coleen Phillimore wrote:
>
> On 2/25/15, 10:59 PM, David Holmes wrote:
>> On 26/02/2015 1:44 PM, Coleen Phillimore wrote:
>>>
>>> Hi, I think you have better reviewer for this now but I did have a
>>> couple of questions.
>>>
>>> in
>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/2-for_jdk9_hs_rt/src/share/vm/runtime/task.cpp.udiff.html
>>>
>>>
>>> This comment is really confusing because I think you do in fact honor
>>> the safepoint protocol sometimes for the WatcherThread, true?
>>
>> Depends what you mean by "honor the safepoint protocol". The
>> WatcherThread will sometimes acquire locks that potentially check the
>> safepoint state, but as it is not a JavaThread such paths are never
>> actually taken. Hence we can call the code that will never consider
>> taking them if the only caller of that code (as in this case) is a
>> non-JavaThread.
>
> So you could theoretically have the watcher thread use the regular
> MutexLocker, which checks for safepoints, and somewhere behind the
> scenes, the safepoint check is elided.

Yes, internally there is one path for JavaThreads and a different path 
for non-JavaThreads. JavaThreads perform thread-state transitions which 
form part of the safepoint protocol.

> So the comment really doesn't explain what's going on, and leads one to
> believe that you need to be careful _not_ to take a safepoint check with
> the Watcher thread.

I don't read it that way but would it be better if it said "do not _need 
to_ honor" instead?

David
-----

> I'll stop reading comments again.
>
> Thanks for the explanation though, the latter part I knew about.
>
> Coleen
>
>>
>>>    65     // The WatcherThread is not a JavaThread so we do not honor
>>> the
>>>    66     // safepoint protocol for the PeriodicTask_lock.
>>>    67     MutexLockerEx ml(PeriodicTask_lock,
>>> Mutex::_no_safepoint_check_flag);
>>>
>>>
>>> Because WatcherThread::stop() does safepoint check.   It's unclear
>>> whether enroll and disenroll can be called by the WatcherThread also.
>>> If so, should they have no_safepoint_check.
>>
>> You can only use MutexLockerEx with _no_safepoint_check_flag if you
>> never want to check for safepoints. If the only caller is a
>> non-JavaThread then that is trivially true. If the caller can be a
>> JavaThread then safepoint checking can only be elided under very
>> specific circumstances (ie leaf methods with other constraints). If
>> the lock is to be acquired by JavaThreads, as is the case with
>> enrol/disenrol then we should check for safepoints (as the conditions
>> for eliding safepoint checks for JavaThreads are not met by those
>> methods).
>>
>>> The whole safepoint checking inconsistency still makes me nervous, but
>>> the comment seems misleading.
>>
>> Hope it is clearer now.
>>
>> David
>>
>>>
>>> I honestly don't know enough about the rest to review it.
>>>
>>> thanks,
>>> Coleen
>>>
>>> On 2/25/15, 12:00 PM, Daniel D. Daugherty wrote:
>>>> This should be the last webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/2-for_jdk9_hs_rt/
>>>>
>>>> Coleen, since you were one of my reviewers on JDK-8047720, I'd like
>>>> to hear from you in this hopefully final round...
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>> On 2/18/15 10:00 AM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> Here is an updated webrev after addressing David H's comments:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/1-for_jdk9_hs_rt/
>>>>>
>>>>> Also, here is the bug's URL:
>>>>>
>>>>> JDK-8072439 fix for 8047720 may need more work
>>>>> https://bugs.openjdk.java.net/browse/JDK-8072439
>>>>>
>>>>> Update for testing: I'm taking the new Remote Build and Test (RBT)
>>>>> system for a ride during its beta period so I won't be doing direct
>>>>> Aurora Adhoc jobs...
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 2/17/15 2:44 PM, Daniel D. Daugherty wrote:
>>>>>> Greetings,
>>>>>>
>>>>>> My fix for the following bug:
>>>>>>
>>>>>>     JDK-8047720 Xprof hangs on Solaris
>>>>>>
>>>>>> that was pushed to JDK9 last June needs to be cleaned up.
>>>>>>
>>>>>> Thanks to Alex Garthwaite (agarthwaite at twitter.com) and Carsten
>>>>>> Varming (varming at gmail.com) for reporting the mess that I made
>>>>>> in WatcherThread::stop() and for suggesting fixes.
>>>>>>
>>>>>> This code review is for a general cleanup pass on PeriodicTask_lock
>>>>>> and some of the surrounding code. This is a targeted review in that
>>>>>> I would like to hear from three groups of people:
>>>>>>
>>>>>> 1) The author and reviewers for:
>>>>>>
>>>>>>    JDK-7127792 Add the ability to change an existing PeriodicTask's
>>>>>>                execution interval
>>>>>>
>>>>>>    Rickard, David H, and Markus G.
>>>>>>
>>>>>> 2) The reviewers for:
>>>>>>
>>>>>>    JDK-8047720 Xprof hangs on Solaris
>>>>>>
>>>>>>    Markus G and Coleen
>>>>>>
>>>>>> 3) Alex and Carsten
>>>>>>
>>>>>>
>>>>>> Here's the webrev URL:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/0-for_jdk9_hs_rt/
>>>>>>
>>>>>> I've attached the original RFR for JDK-8047720 that explains
>>>>>> the original deadlock that was being fixed. Similar testing
>>>>>> will be done with this fix.
>>>>>>
>>>>>> Dan
>>>>>
>>>>>
>>>>>
>>>>
>>>
>