changeset 57308:9f4e98d4dcd4

8235366: ZGC: Kitchensink.java fails in ZBarrier::should_mark_through Reviewed-by: eosterlund, stefank
author pliden
date Tue, 10 Dec 2019 13:12:25 +0100
parents f52cc36158cf
children de30eb1867e3
files src/hotspot/share/gc/z/zAddress.hpp src/hotspot/share/gc/z/zAddress.inline.hpp src/hotspot/share/gc/z/zBarrier.cpp src/hotspot/share/gc/z/zBarrier.hpp src/hotspot/share/gc/z/zBarrier.inline.hpp
diffstat 5 files changed, 123 insertions(+), 39 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/gc/z/zAddress.hpp	Tue Dec 10 11:26:04 2019 +0000
+++ b/src/hotspot/share/gc/z/zAddress.hpp	Tue Dec 10 13:12:25 2019 +0100
@@ -46,6 +46,7 @@
   static bool is_weak_good(uintptr_t value);
   static bool is_weak_good_or_null(uintptr_t value);
   static bool is_marked(uintptr_t value);
+  static bool is_marked_or_null(uintptr_t value);
   static bool is_finalizable(uintptr_t value);
   static bool is_finalizable_good(uintptr_t value);
   static bool is_remapped(uintptr_t value);
--- a/src/hotspot/share/gc/z/zAddress.inline.hpp	Tue Dec 10 11:26:04 2019 +0000
+++ b/src/hotspot/share/gc/z/zAddress.inline.hpp	Tue Dec 10 13:12:25 2019 +0100
@@ -70,6 +70,10 @@
   return value & ZAddressMetadataMarked;
 }
 
+inline bool ZAddress::is_marked_or_null(uintptr_t value) {
+  return is_marked(value) || is_null(value);
+}
+
 inline bool ZAddress::is_finalizable(uintptr_t value) {
   return value & ZAddressMetadataFinalizable;
 }
--- a/src/hotspot/share/gc/z/zBarrier.cpp	Tue Dec 10 11:26:04 2019 +0000
+++ b/src/hotspot/share/gc/z/zBarrier.cpp	Tue Dec 10 13:12:25 2019 +0100
@@ -84,6 +84,17 @@
     ZHeap::heap()->mark_object<follow, finalizable, publish>(good_addr);
   }
 
+  if (finalizable) {
+    // Make the oop finalizable marked/good, instead of normal marked/good.
+    // This is needed because an object might first becomes finalizable
+    // marked by the GC, and then loaded by a mutator thread. In this case,
+    // the mutator thread must be able to tell that the object needs to be
+    // strongly marked. The finalizable bit in the oop exists to make sure
+    // that a load of a finalizable marked oop will fall into the barrier
+    // slow path so that we can mark the object as strongly reachable.
+    return ZAddress::finalizable_good(good_addr);
+  }
+
   return good_addr;
 }
 
@@ -166,25 +177,17 @@
 // Mark barrier
 //
 uintptr_t ZBarrier::mark_barrier_on_oop_slow_path(uintptr_t addr) {
+  assert(during_mark(), "Invalid phase");
+
+  // Mark
   return mark<Follow, Strong, Overflow>(addr);
 }
 
 uintptr_t ZBarrier::mark_barrier_on_finalizable_oop_slow_path(uintptr_t addr) {
-  const uintptr_t good_addr = mark<Follow, Finalizable, Overflow>(addr);
-  if (ZAddress::is_good(addr)) {
-    // If the oop was already strongly marked/good, then we do
-    // not want to downgrade it to finalizable marked/good.
-    return good_addr;
-  }
+  assert(during_mark(), "Invalid phase");
 
-  // Make the oop finalizable marked/good, instead of normal marked/good.
-  // This is needed because an object might first becomes finalizable
-  // marked by the GC, and then loaded by a mutator thread. In this case,
-  // the mutator thread must be able to tell that the object needs to be
-  // strongly marked. The finalizable bit in the oop exists to make sure
-  // that a load of a finalizable marked oop will fall into the barrier
-  // slow path so that we can mark the object as strongly reachable.
-  return ZAddress::finalizable_good(good_addr);
+  // Mark
+  return mark<Follow, Finalizable, Overflow>(addr);
 }
 
 uintptr_t ZBarrier::mark_barrier_on_root_oop_slow_path(uintptr_t addr) {
--- a/src/hotspot/share/gc/z/zBarrier.hpp	Tue Dec 10 11:26:04 2019 +0000
+++ b/src/hotspot/share/gc/z/zBarrier.hpp	Tue Dec 10 13:12:25 2019 +0100
@@ -41,15 +41,15 @@
   static const bool Publish     = true;
   static const bool Overflow    = false;
 
-  static void self_heal(volatile oop* p, uintptr_t addr, uintptr_t heal_addr);
+  template <ZBarrierFastPath fast_path> static void self_heal(volatile oop* p, uintptr_t addr, uintptr_t heal_addr);
 
   template <ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path> static oop barrier(volatile oop* p, oop o);
   template <ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path> static oop weak_barrier(volatile oop* p, oop o);
   template <ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path> static void root_barrier(oop* p, oop o);
 
-  static bool is_null_fast_path(uintptr_t addr);
   static bool is_good_or_null_fast_path(uintptr_t addr);
   static bool is_weak_good_or_null_fast_path(uintptr_t addr);
+  static bool is_marked_or_null_fast_path(uintptr_t addr);
 
   static bool during_mark();
   static bool during_relocate();
--- a/src/hotspot/share/gc/z/zBarrier.inline.hpp	Tue Dec 10 11:26:04 2019 +0000
+++ b/src/hotspot/share/gc/z/zBarrier.inline.hpp	Tue Dec 10 13:12:25 2019 +0100
@@ -32,6 +32,77 @@
 #include "oops/oop.hpp"
 #include "runtime/atomic.hpp"
 
+// A self heal must always "upgrade" the address metadata bits in
+// accordance with the metadata bits state machine, which has the
+// valid state transitions as described below (where N is the GC
+// cycle).
+//
+// Note the subtleness of overlapping GC cycles. Specifically that
+// oops are colored Remapped(N) starting at relocation N and ending
+// at marking N + 1.
+//
+//              +--- Mark Start
+//              | +--- Mark End
+//              | | +--- Relocate Start
+//              | | | +--- Relocate End
+//              | | | |
+// Marked       |---N---|--N+1--|--N+2--|----
+// Finalizable  |---N---|--N+1--|--N+2--|----
+// Remapped     ----|---N---|--N+1--|--N+2--|
+//
+// VALID STATE TRANSITIONS
+//
+//   Marked(N)           -> Remapped(N)
+//                       -> Marked(N + 1)
+//                       -> Finalizable(N + 1)
+//
+//   Finalizable(N)      -> Marked(N)
+//                       -> Remapped(N)
+//                       -> Marked(N + 1)
+//                       -> Finalizable(N + 1)
+//
+//   Remapped(N)         -> Marked(N + 1)
+//                       -> Finalizable(N + 1)
+//
+// PHASE VIEW
+//
+// ZPhaseMark
+//   Load & Mark
+//     Marked(N)         <- Marked(N - 1)
+//                       <- Finalizable(N - 1)
+//                       <- Remapped(N - 1)
+//                       <- Finalizable(N)
+//
+//   Mark(Finalizable)
+//     Finalizable(N)    <- Marked(N - 1)
+//                       <- Finalizable(N - 1)
+//                       <- Remapped(N - 1)
+//
+//   Load(AS_NO_KEEPALIVE)
+//     Remapped(N - 1)   <- Marked(N - 1)
+//                       <- Finalizable(N - 1)
+//
+// ZPhaseMarkCompleted (Resurrection blocked)
+//   Load & Load(ON_WEAK/PHANTOM_OOP_REF | AS_NO_KEEPALIVE) & KeepAlive
+//     Marked(N)         <- Marked(N - 1)
+//                       <- Finalizable(N - 1)
+//                       <- Remapped(N - 1)
+//                       <- Finalizable(N)
+//
+//   Load(ON_STRONG_OOP_REF | AS_NO_KEEPALIVE)
+//     Remapped(N - 1)   <- Marked(N - 1)
+//                       <- Finalizable(N - 1)
+//
+// ZPhaseMarkCompleted (Resurrection unblocked)
+//   Load
+//     Marked(N)         <- Finalizable(N)
+//
+// ZPhaseRelocate
+//   Load & Load(AS_NO_KEEPALIVE)
+//     Remapped(N)       <- Marked(N)
+//                       <- Finalizable(N)
+
+template <ZBarrierFastPath fast_path>
 inline void ZBarrier::self_heal(volatile oop* p, uintptr_t addr, uintptr_t heal_addr) {
   if (heal_addr == 0) {
     // Never heal with null since it interacts badly with reference processing.
@@ -41,12 +112,10 @@
     return;
   }
 
+  assert(!fast_path(addr), "Invalid self heal");
+  assert(fast_path(heal_addr), "Invalid self heal");
+
   for (;;) {
-    if (addr == heal_addr) {
-      // Already healed
-      return;
-    }
-
     // Heal
     const uintptr_t prev_addr = Atomic::cmpxchg((volatile uintptr_t*)p, addr, heal_addr);
     if (prev_addr == addr) {
@@ -54,15 +123,14 @@
       return;
     }
 
-    if (ZAddress::is_good_or_null(prev_addr)) {
-      // No need to heal
+    if (fast_path(prev_addr)) {
+      // Must not self heal
       return;
     }
 
-    // The oop location was healed by another barrier, but it is still not
-    // good or null. Re-apply healing to make sure the oop is not left with
-    // weaker (remapped or finalizable) metadata bits than what this barrier
-    // tried to apply.
+    // The oop location was healed by another barrier, but still needs upgrading.
+    // Re-apply healing to make sure the oop is not left with weaker (remapped or
+    // finalizable) metadata bits than what this barrier tried to apply.
     assert(ZAddress::offset(prev_addr) == ZAddress::offset(heal_addr), "Invalid offset");
     addr = prev_addr;
   }
@@ -70,7 +138,7 @@
 
 template <ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path>
 inline oop ZBarrier::barrier(volatile oop* p, oop o) {
-  uintptr_t addr = ZOop::to_address(o);
+  const uintptr_t addr = ZOop::to_address(o);
 
   // Fast path
   if (fast_path(addr)) {
@@ -81,7 +149,7 @@
   const uintptr_t good_addr = slow_path(addr);
 
   if (p != NULL) {
-    self_heal(p, addr, good_addr);
+    self_heal<fast_path>(p, addr, good_addr);
   }
 
   return ZOop::from_address(good_addr);
@@ -104,7 +172,7 @@
   if (p != NULL) {
     // The slow path returns a good/marked address or null, but we never mark
     // oops in a weak load barrier so we always heal with the remapped address.
-    self_heal(p, addr, ZAddress::remapped_or_null(good_addr));
+    self_heal<fast_path>(p, addr, ZAddress::remapped_or_null(good_addr));
   }
 
   return ZOop::from_address(good_addr);
@@ -132,10 +200,6 @@
   *p = ZOop::from_address(good_addr);
 }
 
-inline bool ZBarrier::is_null_fast_path(uintptr_t addr) {
-  return ZAddress::is_null(addr);
-}
-
 inline bool ZBarrier::is_good_or_null_fast_path(uintptr_t addr) {
   return ZAddress::is_good_or_null(addr);
 }
@@ -144,6 +208,10 @@
   return ZAddress::is_weak_good_or_null(addr);
 }
 
+inline bool ZBarrier::is_marked_or_null_fast_path(uintptr_t addr) {
+  return ZAddress::is_marked_or_null(addr);
+}
+
 inline bool ZBarrier::during_mark() {
   return ZGlobalPhase == ZPhaseMark;
 }
@@ -300,8 +368,11 @@
 }
 
 inline void ZBarrier::keep_alive_barrier_on_oop(oop o) {
+  const uintptr_t addr = ZOop::to_address(o);
+  assert(ZAddress::is_good(addr), "Invalid address");
+
   if (during_mark()) {
-    barrier<is_null_fast_path, mark_barrier_on_oop_slow_path>(NULL, o);
+    mark_barrier_on_oop_slow_path(addr);
   }
 }
 
@@ -309,14 +380,19 @@
 // Mark barrier
 //
 inline void ZBarrier::mark_barrier_on_oop_field(volatile oop* p, bool finalizable) {
-  // The fast path only checks for null since the GC worker
-  // threads doing marking wants to mark through good oops.
   const oop o = *p;
 
   if (finalizable) {
-    barrier<is_null_fast_path, mark_barrier_on_finalizable_oop_slow_path>(p, o);
+    barrier<is_marked_or_null_fast_path, mark_barrier_on_finalizable_oop_slow_path>(p, o);
   } else {
-    barrier<is_null_fast_path, mark_barrier_on_oop_slow_path>(p, o);
+    const uintptr_t addr = ZOop::to_address(o);
+    if (ZAddress::is_good(addr)) {
+      // Mark through good oop
+      mark_barrier_on_oop_slow_path(addr);
+    } else {
+      // Mark through bad oop
+      barrier<is_good_or_null_fast_path, mark_barrier_on_oop_slow_path>(p, o);
+    }
   }
 }