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/11/19 12:53 PM:
> 
> 
> On Mon, Jun 10, 2019 at 10:45 AM Tom Rodriguez <tom.rodriguez at oracle.com 
> <mailto: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.

You can file a bug report at 
https://bugreport.java.com/bugreport/start_form.do and if you want to 
contribute a fix for it the general OpenJDK guidelines are at 
https://openjdk.java.net/contribute.  That doesn't seem to cover the 
recommended process for hotspot such as the use of webrev, so maybe 
someone can provide a better link.

tom

> 
> 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 callingnmethod::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>
>      > <mailto: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/>
>      >     <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.
>      >      >>
>      >
>