8158854: Ensure release_store is paired with load_acquire in lock-free code
authordholmes
Mon, 29 Aug 2016 20:13:45 -0400
changeset 40887 8d35e19f5548
parent 40886 98cb935dc074
child 40888 f09b053be364
child 40889 adf083cbc37f
8158854: Ensure release_store is paired with load_acquire in lock-free code Reviewed-by: shade, dcubed, zgu
hotspot/src/share/vm/classfile/classLoader.hpp
hotspot/src/share/vm/classfile/verifier.cpp
hotspot/src/share/vm/oops/arrayKlass.hpp
hotspot/src/share/vm/oops/arrayKlass.inline.hpp
hotspot/src/share/vm/oops/instanceKlass.cpp
hotspot/src/share/vm/oops/instanceKlass.hpp
hotspot/src/share/vm/oops/instanceKlass.inline.hpp
hotspot/src/share/vm/oops/objArrayKlass.cpp
hotspot/src/share/vm/oops/typeArrayKlass.cpp
hotspot/src/share/vm/runtime/vmStructs.cpp
--- a/hotspot/src/share/vm/classfile/classLoader.hpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/classfile/classLoader.hpp	Mon Aug 29 20:13:45 2016 -0400
@@ -50,12 +50,14 @@
 
 class ClassPathEntry : public CHeapObj<mtClass> {
 private:
-  ClassPathEntry* _next;
+  ClassPathEntry* volatile _next;
 public:
   // Next entry in class path
-  ClassPathEntry* next() const { return _next; }
+  ClassPathEntry* next() const {
+    return (ClassPathEntry*) OrderAccess::load_ptr_acquire(&_next);
+  }
   void set_next(ClassPathEntry* next) {
-    // may have unlocked readers, so write atomically.
+    // may have unlocked readers, so ensure visibility.
     OrderAccess::release_store_ptr(&_next, next);
   }
   virtual bool is_jrt() = 0;
--- a/hotspot/src/share/vm/classfile/verifier.cpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/classfile/verifier.cpp	Mon Aug 29 20:13:45 2016 -0400
@@ -67,12 +67,12 @@
 static volatile jint _is_new_verify_byte_codes_fn = (jint) true;
 
 static void* verify_byte_codes_fn() {
-  if (_verify_byte_codes_fn == NULL) {
+  if (OrderAccess::load_ptr_acquire(&_verify_byte_codes_fn) == NULL) {
     void *lib_handle = os::native_java_library();
     void *func = os::dll_lookup(lib_handle, "VerifyClassCodesForMajorVersion");
     OrderAccess::release_store_ptr(&_verify_byte_codes_fn, func);
     if (func == NULL) {
-      OrderAccess::release_store(&_is_new_verify_byte_codes_fn, false);
+      _is_new_verify_byte_codes_fn = false;
       func = os::dll_lookup(lib_handle, "VerifyClassCodes");
       OrderAccess::release_store_ptr(&_verify_byte_codes_fn, func);
     }
--- a/hotspot/src/share/vm/oops/arrayKlass.hpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/arrayKlass.hpp	Mon Aug 29 20:13:45 2016 -0400
@@ -56,7 +56,9 @@
   void set_dimension(int dimension)     { _dimension = dimension; }
 
   Klass* higher_dimension() const     { return _higher_dimension; }
+  inline Klass* higher_dimension_acquire() const; // load with acquire semantics
   void set_higher_dimension(Klass* k) { _higher_dimension = k; }
+  inline void release_set_higher_dimension(Klass* k); // store with release semantics
   Klass** adr_higher_dimension()      { return (Klass**)&this->_higher_dimension;}
 
   Klass* lower_dimension() const      { return _lower_dimension; }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/src/share/vm/oops/arrayKlass.inline.hpp	Mon Aug 29 20:13:45 2016 -0400
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2016, 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.
+ *
+ */
+
+#ifndef SHARE_VM_OOPS_ARRAYKLASS_INLINE_HPP
+#define SHARE_VM_OOPS_ARRAYKLASS_INLINE_HPP
+
+#include "runtime/orderAccess.inline.hpp"
+#include "oops/arrayKlass.hpp"
+
+inline Klass* ArrayKlass::higher_dimension_acquire() const {
+  return (Klass*) OrderAccess::load_ptr_acquire(&_higher_dimension);
+}
+
+inline void ArrayKlass::release_set_higher_dimension(Klass* k) {
+  OrderAccess::release_store_ptr(&_higher_dimension, k);
+}
+
+#endif // SHARE_VM_OOPS_ARRAYKLASS_INLINE_HPP
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp	Mon Aug 29 20:13:45 2016 -0400
@@ -1041,7 +1041,8 @@
 }
 
 Klass* InstanceKlass::array_klass_impl(instanceKlassHandle this_k, bool or_null, int n, TRAPS) {
-  if (this_k->array_klasses() == NULL) {
+  // Need load-acquire for lock-free read
+  if (this_k->array_klasses_acquire() == NULL) {
     if (or_null) return NULL;
 
     ResourceMark rm;
@@ -1054,7 +1055,8 @@
       // Check if update has already taken place
       if (this_k->array_klasses() == NULL) {
         Klass*    k = ObjArrayKlass::allocate_objArray_klass(this_k->class_loader_data(), 1, this_k, CHECK_NULL);
-        this_k->set_array_klasses(k);
+        // use 'release' to pair with lock-free load
+        this_k->release_set_array_klasses(k);
       }
     }
   }
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp	Mon Aug 29 20:13:45 2016 -0400
@@ -148,7 +148,7 @@
   // Package this class is defined in
   PackageEntry*   _package_entry;
   // Array classes holding elements of this class.
-  Klass*          _array_klasses;
+  Klass* volatile _array_klasses;
   // Constant pool for this class.
   ConstantPool* _constants;
   // The InnerClasses attribute and EnclosingMethod attribute. The
@@ -230,7 +230,7 @@
   OopMapCache*    volatile _oop_map_cache;   // OopMapCache for all methods in the klass (allocated lazily)
   MemberNameTable* _member_names;        // Member names
   JNIid*          _jni_ids;              // First JNI identifier for static fields in this class
-  jmethodID*      _methods_jmethod_ids;  // jmethodIDs corresponding to method_idnum, or NULL if none
+  jmethodID*      volatile _methods_jmethod_ids;  // jmethodIDs corresponding to method_idnum, or NULL if none
   intptr_t        _dep_context;          // packed DependencyContext structure
   nmethod*        _osr_nmethods_head;    // Head of list of on-stack replacement nmethods for this class
 #if INCLUDE_JVMTI
@@ -368,7 +368,9 @@
 
   // array klasses
   Klass* array_klasses() const             { return _array_klasses; }
+  inline Klass* array_klasses_acquire() const; // load with acquire semantics
   void set_array_klasses(Klass* k)         { _array_klasses = k; }
+  inline void release_set_array_klasses(Klass* k); // store with release semantics
 
   // methods
   Array<Method*>* methods() const          { return _methods; }
@@ -1238,10 +1240,8 @@
   // cache management logic if the caches can grow instead of just
   // going from NULL to non-NULL.
   bool idnum_can_increment() const      { return has_been_redefined(); }
-  jmethodID* methods_jmethod_ids_acquire() const
-         { return (jmethodID*)OrderAccess::load_ptr_acquire(&_methods_jmethod_ids); }
-  void release_set_methods_jmethod_ids(jmethodID* jmeths)
-         { OrderAccess::release_store_ptr(&_methods_jmethod_ids, jmeths); }
+  inline jmethodID* methods_jmethod_ids_acquire() const;
+  inline void release_set_methods_jmethod_ids(jmethodID* jmeths);
 
   // Lock during initialization
 public:
--- a/hotspot/src/share/vm/oops/instanceKlass.inline.hpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/instanceKlass.inline.hpp	Mon Aug 29 20:13:45 2016 -0400
@@ -29,10 +29,27 @@
 #include "oops/instanceKlass.hpp"
 #include "oops/klass.hpp"
 #include "oops/oop.inline.hpp"
+#include "runtime/orderAccess.inline.hpp"
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/macros.hpp"
 
+inline Klass* InstanceKlass::array_klasses_acquire() const {
+  return (Klass*) OrderAccess::load_ptr_acquire(&_array_klasses);
+}
+
+inline void InstanceKlass::release_set_array_klasses(Klass* k) {
+  OrderAccess::release_store_ptr(&_array_klasses, k);
+}
+
+inline jmethodID* InstanceKlass::methods_jmethod_ids_acquire() const {
+  return (jmethodID*)OrderAccess::load_ptr_acquire(&_methods_jmethod_ids);
+}
+
+inline void InstanceKlass::release_set_methods_jmethod_ids(jmethodID* jmeths) {
+  OrderAccess::release_store_ptr(&_methods_jmethod_ids, jmeths);
+}
+
 // The iteration over the oops in objects is a hot path in the GC code.
 // By force inlining the following functions, we get similar GC performance
 // as the previous macro based implementation.
--- a/hotspot/src/share/vm/oops/objArrayKlass.cpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/objArrayKlass.cpp	Mon Aug 29 20:13:45 2016 -0400
@@ -34,6 +34,7 @@
 #include "memory/metadataFactory.hpp"
 #include "memory/resourceArea.hpp"
 #include "memory/universe.inline.hpp"
+#include "oops/arrayKlass.inline.hpp"
 #include "oops/instanceKlass.hpp"
 #include "oops/klass.inline.hpp"
 #include "oops/objArrayKlass.inline.hpp"
@@ -42,7 +43,6 @@
 #include "oops/symbol.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/mutexLocker.hpp"
-#include "runtime/orderAccess.inline.hpp"
 #include "utilities/copy.hpp"
 #include "utilities/macros.hpp"
 
@@ -321,7 +321,8 @@
   int dim = dimension();
   if (dim == n) return this;
 
-  if (higher_dimension() == NULL) {
+  // lock-free read needs acquire semantics
+  if (higher_dimension_acquire() == NULL) {
     if (or_null)  return NULL;
 
     ResourceMark rm;
@@ -339,8 +340,8 @@
           ObjArrayKlass::allocate_objArray_klass(class_loader_data(), dim + 1, this, CHECK_NULL);
         ObjArrayKlass* ak = ObjArrayKlass::cast(k);
         ak->set_lower_dimension(this);
-        OrderAccess::storestore();
-        set_higher_dimension(ak);
+        // use 'release' to pair with lock-free load
+        release_set_higher_dimension(ak);
         assert(ak->is_objArray_klass(), "incorrect initialization of ObjArrayKlass");
       }
     }
--- a/hotspot/src/share/vm/oops/typeArrayKlass.cpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/oops/typeArrayKlass.cpp	Mon Aug 29 20:13:45 2016 -0400
@@ -34,6 +34,7 @@
 #include "memory/resourceArea.hpp"
 #include "memory/universe.hpp"
 #include "memory/universe.inline.hpp"
+#include "oops/arrayKlass.inline.hpp"
 #include "oops/instanceKlass.hpp"
 #include "oops/klass.inline.hpp"
 #include "oops/objArrayKlass.hpp"
@@ -41,7 +42,6 @@
 #include "oops/typeArrayKlass.inline.hpp"
 #include "oops/typeArrayOop.inline.hpp"
 #include "runtime/handles.inline.hpp"
-#include "runtime/orderAccess.inline.hpp"
 #include "utilities/macros.hpp"
 
 bool TypeArrayKlass::compute_is_subtype_of(Klass* k) {
@@ -166,7 +166,8 @@
     if (dim == n)
       return this;
 
-  if (higher_dimension() == NULL) {
+  // lock-free read needs acquire semantics
+  if (higher_dimension_acquire() == NULL) {
     if (or_null)  return NULL;
 
     ResourceMark rm;
@@ -181,8 +182,8 @@
               class_loader_data(), dim + 1, this, CHECK_NULL);
         ObjArrayKlass* h_ak = ObjArrayKlass::cast(oak);
         h_ak->set_lower_dimension(this);
-        OrderAccess::storestore();
-        set_higher_dimension(h_ak);
+        // use 'release' to pair with lock-free load
+        release_set_higher_dimension(h_ak);
         assert(h_ak->is_objArray_klass(), "incorrect initialization of ObjArrayKlass");
       }
     }
--- a/hotspot/src/share/vm/runtime/vmStructs.cpp	Mon Aug 29 23:04:48 2016 +0400
+++ b/hotspot/src/share/vm/runtime/vmStructs.cpp	Mon Aug 29 20:13:45 2016 -0400
@@ -242,7 +242,7 @@
   nonstatic_field(ConstantPool,                _reference_map,                                Array<u2>*)                            \
   nonstatic_field(ConstantPoolCache,           _length,                                       int)                                   \
   nonstatic_field(ConstantPoolCache,           _constant_pool,                                ConstantPool*)                         \
-  nonstatic_field(InstanceKlass,               _array_klasses,                                Klass*)                                \
+  volatile_nonstatic_field(InstanceKlass,      _array_klasses,                                Klass*)                                \
   nonstatic_field(InstanceKlass,               _methods,                                      Array<Method*>*)                       \
   nonstatic_field(InstanceKlass,               _default_methods,                              Array<Method*>*)                       \
   nonstatic_field(InstanceKlass,               _local_interfaces,                             Array<Klass*>*)                        \
@@ -271,7 +271,7 @@
   nonstatic_field(InstanceKlass,               _osr_nmethods_head,                            nmethod*)                              \
   JVMTI_ONLY(nonstatic_field(InstanceKlass,    _breakpoints,                                  BreakpointInfo*))                      \
   nonstatic_field(InstanceKlass,               _generic_signature_index,                      u2)                                    \
-  nonstatic_field(InstanceKlass,               _methods_jmethod_ids,                          jmethodID*)                            \
+  volatile_nonstatic_field(InstanceKlass,      _methods_jmethod_ids,                          jmethodID*)                            \
   volatile_nonstatic_field(InstanceKlass,      _idnum_allocated_count,                        u2)                                    \
   nonstatic_field(InstanceKlass,               _annotations,                                  Annotations*)                          \
   nonstatic_field(InstanceKlass,               _method_ordering,                              Array<int>*)                           \