8010196: NPG: Internal Error: Metaspace allocation lock -- possible deadlock

Refactor the CLD dependency list into a separate class. Use an ObjectLocker to synchronize additions to the CLD dependency list.

Reviewed-by: stefank, coleenp

hotspot/src/share/vm/classfile/classLoaderData.cpp

@@ -70,15 +70,19 @@ ClassLoaderData::ClassLoaderData(Handle h_class_loader, bool is_anonymous) :
   _is_anonymous(is_anonymous), _keep_alive(is_anonymous), // initially
   _metaspace(NULL), _unloading(false), _klasses(NULL),
   _claimed(0), _jmethod_ids(NULL), _handles(NULL), _deallocate_list(NULL),
-  _next(NULL), _dependencies(NULL),
+  _next(NULL), _dependencies(),
   _metaspace_lock(new Mutex(Monitor::leaf+1, "Metaspace allocation lock", true)) {
     // empty
 }
 
 void ClassLoaderData::init_dependencies(TRAPS) {
+  _dependencies.init(CHECK);
+}
+
+void ClassLoaderData::Dependencies::init(TRAPS) {
   // Create empty dependencies array to add to. CMS requires this to be
   // an oop so that it can track additions via card marks.  We think.
-  _dependencies = (oop)oopFactory::new_objectArray(2, CHECK);
+  _list_head = oopFactory::new_objectArray(2, CHECK);
 }
 
 bool ClassLoaderData::claim() {
@@ -95,13 +99,17 @@ void ClassLoaderData::oops_do(OopClosure* f, KlassClosure* klass_closure, bool m
   }
 
   f->do_oop(&_class_loader);
-  f->do_oop(&_dependencies);
+  _dependencies.oops_do(f);
   _handles->oops_do(f);
   if (klass_closure != NULL) {
     classes_do(klass_closure);
   }
 }
 
+void ClassLoaderData::Dependencies::oops_do(OopClosure* f) {
+  f->do_oop((oop*)&_list_head);
+}
+
 void ClassLoaderData::classes_do(KlassClosure* klass_closure) {
   for (Klass* k = _klasses; k != NULL; k = k->next_link()) {
     klass_closure->do_klass(k);
@@ -154,14 +162,14 @@ void ClassLoaderData::record_dependency(Klass* k, TRAPS) {
   // It's a dependency we won't find through GC, add it. This is relatively rare
   // Must handle over GC point.
   Handle dependency(THREAD, to);
-  from_cld->add_dependency(dependency, CHECK);
+  from_cld->_dependencies.add(dependency, CHECK);
 }
 
 
-void ClassLoaderData::add_dependency(Handle dependency, TRAPS) {
+void ClassLoaderData::Dependencies::add(Handle dependency, TRAPS) {
   // Check first if this dependency is already in the list.
   // Save a pointer to the last to add to under the lock.
-  objArrayOop ok = (objArrayOop)_dependencies;
+  objArrayOop ok = _list_head;
   objArrayOop last = NULL;
   while (ok != NULL) {
     last = ok;
@@ -184,16 +192,17 @@ void ClassLoaderData::add_dependency(Handle dependency, TRAPS) {
   objArrayHandle new_dependency(THREAD, deps);
 
   // Add the dependency under lock
-  locked_add_dependency(last_handle, new_dependency);
+  locked_add(last_handle, new_dependency, THREAD);
 }
 
-void ClassLoaderData::locked_add_dependency(objArrayHandle last_handle,
-                                            objArrayHandle new_dependency) {
+void ClassLoaderData::Dependencies::locked_add(objArrayHandle last_handle,
+                                               objArrayHandle new_dependency,
+                                               Thread* THREAD) {
 
   // Have to lock and put the new dependency on the end of the dependency
   // array so the card mark for CMS sees that this dependency is new.
   // Can probably do this lock free with some effort.
-  MutexLockerEx ml(metaspace_lock(),  Mutex::_no_safepoint_check_flag);
+  ObjectLocker ol(Handle(THREAD, _list_head), THREAD);
 
   oop loader_or_mirror = new_dependency->obj_at(0);
 

hotspot/src/share/vm/classfile/classLoaderData.hpp

@@ -93,6 +93,18 @@ class ClassLoaderDataGraph : public AllStatic {
 class ClassLoaderData : public CHeapObj<mtClass> {
   friend class VMStructs;
  private:
+  class Dependencies VALUE_OBJ_CLASS_SPEC {
+    objArrayOop _list_head;
+    void locked_add(objArrayHandle last,
+                    objArrayHandle new_dependency,
+                    Thread* THREAD);
+   public:
+    Dependencies() : _list_head(NULL) {}
+    void add(Handle dependency, TRAPS);
+    void init(TRAPS);
+    void oops_do(OopClosure* f);
+  };
+
   friend class ClassLoaderDataGraph;
   friend class ClassLoaderDataGraphMetaspaceIterator;
   friend class MetaDataFactory;
@@ -100,10 +112,11 @@ class ClassLoaderData : public CHeapObj<mtClass> {
 
   static ClassLoaderData * _the_null_class_loader_data;
 
-  oop _class_loader;       // oop used to uniquely identify a class loader
-                           // class loader or a canonical class path
-  oop _dependencies;       // oop to hold dependencies from this class loader
-                           // data to others.
+  oop _class_loader;          // oop used to uniquely identify a class loader
+                              // class loader or a canonical class path
+  Dependencies _dependencies; // holds dependencies from this class loader
+                              // data to others.
+
   Metaspace * _metaspace;  // Meta-space where meta-data defined by the
                            // classes in the class loader are allocated.
   Mutex* _metaspace_lock;  // Locks the metaspace for allocations and setup.
@@ -134,9 +147,6 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   static Metaspace* _ro_metaspace;
   static Metaspace* _rw_metaspace;
 
-  void add_dependency(Handle dependency, TRAPS);
-  void locked_add_dependency(objArrayHandle last, objArrayHandle new_dependency);
-
   void set_next(ClassLoaderData* next) { _next = next; }
   ClassLoaderData* next() const        { return _next; }
 

hotspot/test/gc/metaspace/G1AddMetaspaceDependency.java

@@ -0,0 +1,123 @@
+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test G1AddMetaspaceDependency
+ * @bug 8010196
+ * @summary Checks that we don't get locking problems when adding metaspace dependencies with the G1 update buffer monitor
+ * @run main/othervm -XX:+UseG1GC -XX:G1UpdateBufferSize=1 G1AddMetaspaceDependency
+ */
+
+import java.io.InputStream;
+
+public class G1AddMetaspaceDependency {
+
+  static byte[] getClassBytes(String name) {
+    byte[] b = null;
+    try (InputStream is = ClassLoader.getSystemResourceAsStream(name)) {
+      byte[] tmp = new byte[is.available()];
+      is.read(tmp);
+      b = tmp;
+    } finally {
+      if (b == null) {
+        throw new RuntimeException("Unable to load class file");
+      }
+      return b;
+    }
+  }
+
+  static final String a_name = G1AddMetaspaceDependency.class.getName() + "$A";
+  static final String b_name = G1AddMetaspaceDependency.class.getName() + "$B";
+
+  public static void main(String... args) throws Exception {
+    final byte[] a_bytes = getClassBytes(a_name + ".class");
+    final byte[] b_bytes = getClassBytes(b_name + ".class");
+
+    for (int i = 0; i < 1000; i += 1) {
+      runTest(a_bytes, b_bytes);
+    }
+  }
+
+  static class Loader extends ClassLoader {
+    private final String myClass;
+    private final byte[] myBytes;
+    private final String friendClass;
+    private final ClassLoader friendLoader;
+
+    Loader(String myClass, byte[] myBytes,
+           String friendClass, ClassLoader friendLoader) {
+      this.myClass = myClass;
+      this.myBytes = myBytes;
+      this.friendClass = friendClass;
+      this.friendLoader = friendLoader;
+    }
+
+    Loader(String myClass, byte[] myBytes) {
+      this(myClass, myBytes, null, null);
+    }
+
+    @Override
+    public Class<?> loadClass(String name) throws ClassNotFoundException {
+      Class<?> c = findLoadedClass(name);
+      if (c != null) {
+        return c;
+      }
+
+      if (name.equals(friendClass)) {
+        return friendLoader.loadClass(name);
+      }
+
+      if (name.equals(myClass)) {
+        c = defineClass(name, myBytes, 0, myBytes.length);
+        resolveClass(c);
+        return c;
+      }
+
+      return findSystemClass(name);
+    }
+
+  }
+
+  private static void runTest(final byte[] a_bytes, final byte[] b_bytes) throws Exception {
+    Loader a_loader = new Loader(a_name, a_bytes);
+    Loader b_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+    Loader c_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+    Loader d_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+    Loader e_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+    Loader f_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+    Loader g_loader = new Loader(b_name, b_bytes, a_name, a_loader);
+
+    byte[] b = new byte[20 * 2 << 20];
+    Class<?> c;
+    c = b_loader.loadClass(b_name);
+    c = c_loader.loadClass(b_name);
+    c = d_loader.loadClass(b_name);
+    c = e_loader.loadClass(b_name);
+    c = f_loader.loadClass(b_name);
+    c = g_loader.loadClass(b_name);
+  }
+  public class A {
+  }
+  class B extends A {
+  }
+}