review (S) for 6965671: fatal error: acquiring lock JNIGlobalHandle_lock/16 out of order with lock CodeCache_lock/1

[Leela Mohan]

On Mon, Jun 10, 2019 at 10:45 AM Tom Rodriguez <tom.rodriguez at oracle.com>
wrote:

>
>
> Leela Mohan wrote on 6/7/19 6:39 PM:
> > This is super late reply to this review mail but i have couple of
> > questions about this change.
>
> Wow 9 years.  :)
>
> >
> > Looking at the diff here :
> > http://hg.openjdk.java.net/jdk7/jdk7/hotspot/rev/65b0c03b165d
> >
> > I think, Calling JvmtiExport::post_compiled_method_load(JvmtiEnv*
> > env,..) to JvmtiExport::post_compiled_method_load(nmethod*) versio is
> > possibly incorrect since, this would do callbacks to multiple jvmti
> > environments (jvmti agents) which is unexpected. Callbacks are expected
> > to be called to current jvmti env which called GenerateEvents.
>
> I think you're right.  Anyone listening for those events will also get
> notified which isn't quite right.  Please file a bug.  It should be easy
> enough to fix.  Factoring out the notification body of
> post_compiled_method_load into it's own method and calling that from
> generate_compiled_method_load_events would work great.
>

Thanks for confirming. I don't know the formalities of filing a bug. Can
you please pass me the instructions. Thanks.

And I was wondering what was the original lock rank ordering issue. It took
some time for me to understand what the issue is.
Before the change, CodeCache_lock was held when  nmethod closure  (i.e)
nmethodCollector::do_nmethod(nmethod* nm) was called on each nmethod and
closure was calling nmethod::get_and_cache_jmethod_id() which needed
JNIGlobalHandle_lock (jmethods then were jweak handles).  Lot seemed to
have changed after this but intent of not holding CodeCache_lock around
nmethod::get_and_cache_jmethod_id() is still valid.

Thanks,
Leela


> tom
>
> >
> >
> >
> >
> >
> > On Thu, Jul 1, 2010 at 4:07 PM Tom Rodriguez <tom.rodriguez at oracle.com
> > <mailto:tom.rodriguez at oracle.com>> wrote:
> >
> >     Thanks.  Dan also reviewed this as I was developing it.
> >
> >     tom
> >
> >     On Jul 1, 2010, at 2:55 PM, Vladimir Kozlov wrote:
> >
> >      > Thanks, Tom
> >      >
> >      > Changes are good to go.
> >      >
> >      > Vladimir
> >      >
> >      > On 7/1/10 2:41 PM, Tom Rodriguez wrote:
> >      >>
> >      >> On Jul 1, 2010, at 2:36 PM, Vladimir Kozlov wrote:
> >      >>
> >      >>> You did not explain your changes for cb->name().
> >      >>
> >      >> That was just a clean up I've been wanting to do.  There was a
> >     time then only some of the CodeBlob subclasses had name() methods
> >     but now it's part of CodeBlob so that logic is unnecessary.
> >      >>
> >      >>> Why we don't lock nmethod in sweeper during CodeCache walk but
> >     lock here?
> >      >>
> >      >> nmethodLocker protects you from the sweeper so the sweeper
> >     doesn't need protection.
> >      >>
> >      >> tom
> >      >>
> >      >>>
> >      >>> Thanks,
> >      >>> Vladimir
> >      >>>
> >      >>> On 7/1/10 1:58 PM, Tom Rodriguez wrote:
> >      >>>> http://cr.openjdk.java.net/~never/6965671/
> >     <http://cr.openjdk.java.net/%7Enever/6965671/>
> >      >>>>
> >      >>>> 6965671: fatal error: acquiring lock JNIGlobalHandle_lock/16
> >     out of order with lock CodeCache_lock/1
> >      >>>> Reviewed-by:
> >      >>>>
> >      >>>> The jmethodID fix created a deadlock because of lock ordering
> when
> >      >>>> walking the CodeCache to generate CompiledMethodLoad events.
> >     The walk
> >      >>>> is performed with the CodeCache_lock held but we need to
> >     acquire the
> >      >>>> JNIGlobalHandle_lock to make a jmethodID.  I considered
> >     switching back
> >      >>>> to collecting the methodOop instead of the jmethodID under the
> >     lock
> >      >>>> but inspection of the code made it clear that there's a
> >     preexisting
> >      >>>> problem with GC of the methodOops stored into the nmethodDesc.
> >      >>>> Instead I switched to a simpler implementation that holds the
> >      >>>> CodeCache_lock while walking the CodeCache but releases the
> >     lock to
> >      >>>> actually perform the notify.  This simplifies the code greatly
> >     as well
> >      >>>> as fixing the bug.
> >      >>>>
> >      >>>> Tested with NSK jvmti,jdi,jdb,hprof,jit,regression and
> >     JDI_REGRESSION
> >      >>>> tests.
> >      >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190611/a7afd30e/attachment.html>