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

[Markus Gronlund]

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.

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.

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
>