RFR (S/M) JDK-8010196 NPG: Internal Error: Metaspace allocation lock -- possible deadlock

[David Holmes]

Hi Mikael,

This approach sounds (and looks) reasonable to me, but I'll leav eit to 
others to officially Review.

One concern I did have was whether the initialization of the array could 
fail and what that would mean for how this particular cld was used. That 
led me to the following code which looks suspect to me:

// ClassLoaderData into the java/lang/ClassLoader object as a hidden field
ClassLoaderData* ClassLoaderDataGraph::add(ClassLoaderData** cld_addr, 
Handle loader, TRAPS) {
   ...
   ClassLoaderData* cld = new ClassLoaderData(loader, is_anonymous);

   if (cld_addr != NULL) {
     // First, Atomically set it
     ClassLoaderData* old = (ClassLoaderData*) Atomic::cmpxchg_ptr(cld, 
cld_addr, NULL);
     if (old != NULL) {
       delete cld;
       // Returns the data.
       return old;
     }
   }

   // We won the race, and therefore the task of adding the data to the 
list of
   // class loader data
   do {
     cld->set_next(next);
     ClassLoaderData* exchanged = 
(ClassLoaderData*)Atomic::cmpxchg_ptr(cld, list_head, next);
     if (exchanged == next) {
       ...
       // Create dependencies after the CLD is added to the list. 
Otherwise,
       // the GC GC will not find the CLD and the _class_loader field will
       // not be updated.
       cld->init_dependencies(CHECK_NULL);
       return cld;
     }
     next = exchanged;
   } while (true);
}

So it explicitly states that we create dependencies after adding to the 
list, but if creating the dependencies fails we return null, but leave 
the new cld in the list. Is that a valid thing to do ??

David
-----

On 8/04/2013 11:49 PM, Mikael Gerdin wrote:
> Hi
>
> The problem is that when running the G1 garbage collector a call to
> objArrayOopDesc::obj_at_put can in rare cases cause the G1 dirty card
> queue buffer to fill up and this will cause G1 to take
> DirtyCardQ_CBL_mon/16 to return the full buffer and get a new one.
>
> Adding dependencies to a ClassLoaderData is currently protected by the
> per-metaspace "Metaspace allocation lock"/5 (which protects more than
> just allocations).
>
> Because I want to avoid messing around with the lock ordering I suggest
> that we use an ObjectLocker on the _dependencies oop in ClassLoaderData.
> _dependencies is a 2-element objArrayOop, in effect it's an ad-hoc
> linked list of ClassLoaders/Classes which must be kept alive by this CLD.
>
> Using an ObjectLocker on the head element of the linked list should be
> at least as good as using the metaspace_lock(). There should not be any
> new deadlock issues with VM mutexes since any application thread could
> use a similar synchronized linked list construct.
>
> As a bonus I suggest that we factor out the dependency handling from
> ClassLoaderData into a inner class: ClassLoaderData::Dependencies and
> let Dependencies manage the head of the linked list by itself. This
> should make it more clear that the dependency list is not guarded by the
> metaspace lock anymore.
>
> Buglinks:
> https://jbs.oracle.com/bugs/browse/JDK-8010196
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010196
>
> Testing:
> default jprt run
> jdk/test/java/lang/invoke/BigArityTest.java (which could reproduce the
> issue with -XX:+UseG1GC -XX:G1UpdateBufferSize=1)
>
> Webrevs:
> Incremental:
> http://cr.openjdk.java.net/~mgerdin/8010196/webrev.0-fix/webrev
> http://cr.openjdk.java.net/~mgerdin/8010196/webrev.0-factor-out/webrev
> http://cr.openjdk.java.net/~mgerdin/8010196/webrev.0-rename/webrev
> Everything:
> http://cr.openjdk.java.net/~mgerdin/8010196/webrev.0/webrev
>
> Thanks
> /Mikael