Markus,
It would be helpful if you could find a place to record a comment about the
JfrStream_lock - that the rank needs to
be between safe point and JfrBuffer_lock. That way, should we do cleanup in
this area we will know the constraints.
thanks,
Karen
> On Feb 24, 2016, at 2:30 PM, Karen Kinnear <karen.kinn...@oracle.com> wrote:
>
> Markus,
>
> See below.
>> On Feb 23, 2016, at 11:58 PM, David Holmes <david.hol...@oracle.com> wrote:
>>
>> Hi Markus,
>>
>> On 23/02/2016 8:00 PM, Markus Gronlund wrote:
>>> Thanks for taking a look David,
>>>
>>> I have verified the new lock rankings by running Kitchensink and by code
>>> inspection.
>>>
>>> The original intent was to try to make the tracing locks transparent much
>>> like the way ttyLocker is today ("event" ranking). I did this in an effort
>>> to provide "seamless" tracing usages from higher ranking locks (leafs in
>>> particular). As you point out, the mechanics for accomplishing this becomes
>>> a bit of a force-fit in light of the existing lock_types enum. I still
>>> needed lock ordering since ttyLocker is being used under some of these
>>> locks; in addition, there is a relative order between two of these locks
>>> (that was the reason for special+1 - and yes it lands on the same ordinal
>>> as suspend_resume (but I didn't want to use that designation since it is
>>> totally separate)).
>>>
>>> I also elaborated on expanding the lock_types enum:
>>>
>>> enum lock_types {
>>> event,
>>> special,
>>> suspend_resume,
>>> traceleaf,
>>> trace,
>>> leaf,
>>> safepoint = leaf + 10,
>>> barrier = safepoint + 1,
>>> nonleaf = barrier + 1,
>>> max_nonleaf = nonleaf + 900,
>>> native = max_nonleaf + 1
>>> };
>>>
>>> This seems also to work but I am not sure about any disruptions this might
>>> cause. Would mean much more investigation to do any changes here.
>>
>> I don't think this should cause any disruptions unless we have hard-coded
>> the enum values somewhere - which we do not seem to do. It also avoids the
>> potential problem of having a lock with rank special+1 being considered to
>> have rank suspend_resume in the rank checking code:
>>
>> if (this->rank() != Mutex::native &&
>> this->rank() != Mutex::suspend_resume &&
>> locks != NULL && locks->rank() <= this->rank() &&
>> ...
>>
>>> On the topic of the "special" rank comment mentioning guarantees not to
>>> block, that would be true for JfrBuffer_lock, but not for JfrStream_lock
>>> (could be blocked on the former). So this is not correct - thanks for
>>> pointing this out.
>>>
>>> I am having a rethink about this:
>>>
>>> Reducing the lock rankings underneath the ordinary leaf ranking would be
>>> for facility yes. However, doing so might cause an unwanted effect:
>>>
>>> It would become easier to generated trace events while holding these other
>>> locks. A better pattern for overall performance would be to instead save
>>> the necessary information, release the lock as soon as possible, and
>>> generate an event post-lock release (if possible). I have not yet seen a
>>> real case where this is not possible to do - I might need to revisit this
>>> if that becomes a real problem in the future.
>>>
>>> Even though it might not be the primary vehicle for enforcement, we could
>>> capitalize on the lock ordering scheme to avoid generating events
>>> underneath other locks unnecessarily.
>>>
>>> So I am retracting the lock ranking "special" suggestion.
>>>
>>> What I actually only want/need is this:
>>>
>>> - def(JfrStream_lock , Mutex, nonleaf, true,
>>> Monitor::_safepoint_check_never);
>>> + def(JfrStream_lock , Mutex, leaf+1, true,
>>> Monitor::_safepoint_check_never);
>>>
>>> The lock ordering assertions will today handle nonleaf for this lock, but
>>> only because of the assertion bailing on
>>> SafepointSynchronize::is_at_safepoint(). I would like to reduce the ranking
>>> from nonleaf to leaf+1 to gracefully handle
>>> SafepointSynchronize::is_synchronizing() and other states as well.
>>
>> I didn't quite follow all that. :) If this simpler suggestion meets your
>> requirements and adheres to the intent of the lock-ranking protocol then
>> that is good.
> I think what you are saying is that actually you do not adhere to the
> lock-ranking protocol. Rather that, when not at a safe point you adhere to the
> lock ranking protocol, but when at a safe point, the safe point check
> (is_a_safepoint) skips the lock ranking check.
> I think the rest of your comment is that with the new tracepoints in the safe
> point code, which need this lock while holding the Safepoint_lock (i.e. while
> getting
> to a safepoint or leaving a safe point but not during a safe point), so you
> need to ensure that both the JfrBuffer_lock and JfrStream_lock can be acquired
> while not actually at a safe point, and while doing a lock ordering check,
> while holding the Safepoint_lock. And you are maintaining the relative
> relationship of JfrBuffer_lock with JfrStream_lock.
>
> So you are just changing JfrStream_lock from non leaf to leaf+1.
>
> If that is what you are saying, I am good with the change. Ship it once
> sufficiently tested.
>
> thanks,
> Karen
>
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks for your feedback
>>> Markus
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes
>>> Sent: den 22 februari 2016 02:10
>>> To: Markus Gronlund; serviceability-dev@openjdk.java.net
>>> Subject: Re: RFR(XXS): 8149803: Adjust lock rankings for some Event-based
>>> tracing locks
>>>
>>> Hi Markus,
>>>
>>> On 20/02/2016 1:55 AM, Markus Gronlund wrote:
>>>> Greetings,
>>>>
>>>> Please review this small change lowering the lock rankings of some locks.
>>>
>>> Have we actually verified the new ranking constraints (ie that special
>>> guarantees not to block) by code inspection?
>>>
>>>> This is done in order to reduce the risk for potential deadlocks and
>>>> to increase the surface area for event generation.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149803
>>>>
>>>> Patch of this tiny change is attached.
>>>
>>> - def(JfrStream_lock , Mutex, nonleaf, true,
>>> Monitor::_safepoint_check_never);
>>> + def(JfrStream_lock , Mutex, special+1, true,
>>> Monitor::_safepoint_check_never);
>>>
>>>
>>> Not clear what "special+1" is supposed to signify here - doesn't that make
>>> it the same rank as suspend_resume?
>>>
>>> enum lock_types {
>>> event,
>>> special,
>>> suspend_resume,
>>> leaf = suspend_resume + 2,
>>> safepoint = leaf + 10,
>>> barrier = safepoint + 1,
>>> nonleaf = barrier + 1,
>>> max_nonleaf = nonleaf + 900,
>>> native = max_nonleaf + 1
>>> };
>>>
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> Thanks in advance
>>>>
>>>> Markus
