RFR(XXS): 8149803: Adjust lock rankings for some Event-based tracing locks

[David Holmes]

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.

Thanks,
David
-----

> Thanks for your feedback
> Markus
>
>
> -----Original Message-----
> From: David Holmes
> Sent: den 22 februari 2016 02:10
> To: Markus Gronlund; serviceability-dev at 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
>>