[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: Preliminary review for 8028107: Kitchensink crashed with EAV
From:       Vladimir Kozlov <vladimir.kozlov () oracle ! com>
Date:       2013-11-28 22:55:56
Message-ID: 5297C9FC.6060405 () oracle ! com
[Download RAW message or body]

Thank you, Mikael

It simplifies my work. I will concentrate on safely bailout from situation when \
nmethod is unloaded. We need to call c2i  adapter instead which should be present at \
that time.

Regards,
Vladimir

On 11/28/13 1:39 AM, Mikael Gerdin wrote:
> Vladimir,
> 
> On Wednesday 27 November 2013 18.49.14 Vladimir Kozlov wrote:
> > Additional note. The code does "lock" nmethod (which is actually the field
> > nm:_lock_count increment):
> > 
> > nmethod* callee_nm = callee_method->code();
> > nmethodLocker nl_callee(callee_nm);
> > 
> > but unloading code does not check it (is_locked_by_vm()) because it is too
> > late anyway.
> > 
> > John Rose suggested to modify nmethodLocker to put related metadata (method,
> > klass) or JavaMirror into thread's handles area (or other reachable from GC
> > place) so that such nmethods will be reachable during GC and be strong
> > roots. It will prevent GC modify nmethod state when a java thread works
> > with nmethod.
> 
> I read through your original explanation below and from a GC perspective your
> explanation seems plausible. I don't really know how compiledICs work, but it
> is possible that an nmethod can be unloaded at a safepoint if any of the
> objects it references are not live. I've had problems with this when working
> on class unloading for G1 previously.
> 
> I don't think it would be enough to create handles for the nmethod's metadata
> since the GC does not actually follow any pointers to find nmethods.
> As far as I understand from the sources nmethod unloading is _forced_ if any
> java object which is embedded in an nmethod is dead.
> If you want to make sure that the nmethod is not unloaded then you would need
> to register your nmethod to a global list and change all the gc code to call
> oops_do on the nmethods on this list with the correct closures, basically some
> sort of nmethod handle stack. I'd prefer to avoid this approach since it will
> add more complexity to GC root scanning and would further complicate class
> unloading for G1.
> 
> I would also like to add that since this nmethod is the only referent of the
> java object that caused the unloading it is highly likely that it will be
> unloaded at the next full gc unless it's on a thread stack. Keeping the
> nmethod alive here seems to only delay the inevitable.
> 
> /Mikael
> 
> > 
> > Thanks,
> > Vladimir
> > 
> > On 11/27/13 5:51 PM, Vladimir Kozlov wrote:
> > > https://bugs.openjdk.java.net/browse/JDK-8028107
> > > 
> > > It is very rare case. Last time it was 1 year ago:
> > > 
> > > https://bugs.openjdk.java.net/browse/JDK-7199505
> > > 
> > > nmethod unloading (nmethod::make_unloaded()) happens only during safepoint
> > > when it contains pointers (embedded or in oop map, relocation info) to
> > > dead java objects (see nmethod::can_unload()).
> > > 
> > > It can only happens when a nmethod is NOT on call stack. GCs have path
> > > over nmethods on stack and they are treated as strong roots. Igor and I
> > > looked on GC code and verified it.
> > > 
> > > I instrumented nmethod::make_unloaded() to see if we may unloading
> > > nmethods on stack and ran kitchensink for few day. No failures.
> > > 
> > > The only place left is compiled inline caches. We DO clean up them at the
> end of safepoint if unloading happens:
> > > if (is_in_use()) {
> > > 
> > > // Transitioning directly from live to unloaded -- so
> > > // we need to force a cache clean-up; remember this
> > > // for later on.
> > > CodeCache::set_needs_cache_clean(true);
> > > 
> > > }
> > > 
> > > But there is only place where we may create new compiledIC after safepoint
> > > when we are resolving call from compiled code:
> > > SharedRuntime::resolve_sub_helper().
> > > 
> > > In this method we record nmethod in local var:
> > > CompiledICInfo virtual_call_info;
> > > 
> > > CompiledIC::compute_monomorphic_entry(callee_method, h_klass,
> > > 
> > > is_optimized, static_bound, virtual_call_info,
> > > CHECK_(methodHandle()));
> > > 
> > > then we take a lock (which is safepoint):
> > > // grab lock, check for deoptimization and potentially patch caller
> > > {
> > > 
> > > MutexLocker ml_patch(CompiledIC_lock);
> > > 
> > > and then create compiledIC based on info in CompiledICInfo:
> > > CompiledIC* inline_cache = CompiledIC_before(caller_nm,
> > > caller_frame.pc());
> > > if (inline_cache->is_clean()) {
> > > 
> > > inline_cache->set_to_monomorphic(virtual_call_info);
> > > 
> > > }
> > > 
> > > at this point nmethod could be unloaded already during above
> > > lock/safepoint. But call site is patched and will be used to call this
> > > nmethod.
> > > 
> > > Note, we can't compute entry point under the same lock because:
> > > // Compute entry points. This might require generation of C2I converter
> > > // frames, so we cannot be holding any locks here. Furthermore, the
> > > // computation of the entry points is independent of patching the call.
> > > 
> > > Please, comment or find any flaws in my logic. The bug is impossible to
> > > reproduce.
> > > 
> > > I am suggesting next fix.
> > > 
> > > src/share/vm/runtime/sharedRuntime.cpp
> > > @@ -1258,12 +1258,14 @@
> > > 
> > > // Now that we are ready to patch if the Method* was redefined then
> > > // don't update call site and let the caller retry.
> > > 
> > > -
> > > -    if (!callee_method->is_old()) {
> > > +    // Don't update call site if nmethod was unloaded during safepoint
> > > +    // which may happened during locking above.
> > > +    if (!callee_method->is_old() && (callee_method->code() == callee_nm))
> > > {>
> > > #ifdef ASSERT
> > > 
> > > // We must not try to patch to jump to an already unloaded method.
> > > if (dest_entry_point != 0) {
> > > 
> > > -        assert(CodeCache::find_blob(dest_entry_point) != NULL,
> > > +        CodeBlob* cb = CodeCache::find_blob(dest_entry_point);
> > > +        assert((cb != NULL) && cb->is_nmethod() &&
> > > ((nmethod*)cb)->is_in_use(),>
> > > "should not unload nmethod while locked");
> > > 
> > > }
> > > 
> > > #endif
> > > 
> > > Thanks,
> > > Vladimir
> 


[prev in list] [next in list] [prev in thread] [next in thread]