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

[Tom Rodriguez]

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.

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