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@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
