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

[Leela Mohan]

Submitted a bug report and here is the  internal review ID : 9061251.

On Wed, Jun 12, 2019 at 5:24 PM Tom Rodriguez <tom.rodriguez at oracle.com>
wrote:

>
>
> 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.
> >      >      >>
> >      >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190616/d87847c3/attachment.html>