changeset 49608:1852b17b0efc

8196485: FromCardCache default card index can cause crashes Summary: The default value of -1 for 32 bit card indices is a regular card value at the border of 2TB heap addresses in the from card cache, so G1 may loose remembered set entries. Extend from card cache entries to 64 bits. Reviewed-by: shade, sjohanss Contributed-by: Thomas Schatzl <thomas.schatzl@oracle.com>, Jarkko Miettinen <jarkko.miettinen@relex.fi>
author tschatzl
date Mon, 26 Mar 2018 16:51:43 +0200
parents acffe6ff3ae7
children cc63a8331f91
files src/hotspot/share/gc/g1/g1FromCardCache.cpp src/hotspot/share/gc/g1/g1FromCardCache.hpp src/hotspot/share/gc/g1/heapRegionRemSet.cpp src/hotspot/share/gc/g1/heapRegionRemSet.hpp test/hotspot/jtreg/gc/g1/TestFromCardCacheIndex.java
diffstat 5 files changed, 150 insertions(+), 41 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/gc/g1/g1FromCardCache.cpp	Mon Mar 26 16:51:43 2018 +0200
+++ b/src/hotspot/share/gc/g1/g1FromCardCache.cpp	Mon Mar 26 16:51:43 2018 +0200
@@ -28,9 +28,9 @@
 #include "memory/padded.inline.hpp"
 #include "utilities/debug.hpp"
 
-int**  G1FromCardCache::_cache = NULL;
-uint   G1FromCardCache::_max_regions = 0;
-size_t G1FromCardCache::_static_mem_size = 0;
+uintptr_t** G1FromCardCache::_cache = NULL;
+uint        G1FromCardCache::_max_regions = 0;
+size_t      G1FromCardCache::_static_mem_size = 0;
 #ifdef ASSERT
 uint   G1FromCardCache::_max_workers = 0;
 #endif
@@ -43,9 +43,9 @@
 #ifdef ASSERT
   _max_workers = num_par_rem_sets;
 #endif
-  _cache = Padded2DArray<int, mtGC>::create_unfreeable(_max_regions,
-                                                       num_par_rem_sets,
-                                                       &_static_mem_size);
+  _cache = Padded2DArray<uintptr_t, mtGC>::create_unfreeable(_max_regions,
+                                                             num_par_rem_sets,
+                                                             &_static_mem_size);
 
   invalidate(0, _max_regions);
 }
@@ -68,7 +68,7 @@
 void G1FromCardCache::print(outputStream* out) {
   for (uint i = 0; i < G1RemSet::num_par_rem_sets(); i++) {
     for (uint j = 0; j < _max_regions; j++) {
-      out->print_cr("_from_card_cache[%u][%u] = %d.",
+      out->print_cr("_from_card_cache[%u][%u] = " SIZE_FORMAT ".",
                     i, j, at(i, j));
     }
   }
--- a/src/hotspot/share/gc/g1/g1FromCardCache.hpp	Mon Mar 26 16:51:43 2018 +0200
+++ b/src/hotspot/share/gc/g1/g1FromCardCache.hpp	Mon Mar 26 16:51:43 2018 +0200
@@ -37,7 +37,7 @@
   // This order minimizes the time to clear all entries for a given region during region
   // freeing. I.e. a single clear of a single memory area instead of multiple separate
   // accesses with a large stride per region.
-  static int** _cache;
+  static uintptr_t** _cache;
   static uint _max_regions;
   static size_t _static_mem_size;
 #ifdef ASSERT
@@ -50,16 +50,14 @@
 #endif
 
  public:
-  enum {
-    InvalidCard = -1 // Card value of an invalid card, i.e. a card index not otherwise used.
-  };
+  static const uintptr_t InvalidCard = UINTPTR_MAX;
 
   static void clear(uint region_idx);
 
   // Returns true if the given card is in the cache at the given location, or
   // replaces the card at that location and returns false.
-  static bool contains_or_replace(uint worker_id, uint region_idx, int card) {
-    int card_in_cache = at(worker_id, region_idx);
+  static bool contains_or_replace(uint worker_id, uint region_idx, uintptr_t card) {
+    uintptr_t card_in_cache = at(worker_id, region_idx);
     if (card_in_cache == card) {
       return true;
     } else {
@@ -68,12 +66,12 @@
     }
   }
 
-  static int at(uint worker_id, uint region_idx) {
+  static uintptr_t at(uint worker_id, uint region_idx) {
     DEBUG_ONLY(check_bounds(worker_id, region_idx);)
     return _cache[region_idx][worker_id];
   }
 
-  static void set(uint worker_id, uint region_idx, int val) {
+  static void set(uint worker_id, uint region_idx, uintptr_t val) {
     DEBUG_ONLY(check_bounds(worker_id, region_idx);)
     _cache[region_idx][worker_id] = val;
   }
--- a/src/hotspot/share/gc/g1/heapRegionRemSet.cpp	Mon Mar 26 16:51:43 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionRemSet.cpp	Mon Mar 26 16:51:43 2018 +0200
@@ -93,17 +93,8 @@
     // If the test below fails, then this table was reused concurrently
     // with this operation.  This is OK, since the old table was coarsened,
     // and adding a bit to the new table is never incorrect.
-    // If the table used to belong to a continues humongous region and is
-    // now reused for the corresponding start humongous region, we need to
-    // make sure that we detect this. Thus, we call is_in_reserved_raw()
-    // instead of just is_in_reserved() here.
     if (loc_hr->is_in_reserved(from)) {
-      size_t hw_offset = pointer_delta((HeapWord*)from, loc_hr->bottom());
-      CardIdx_t from_card = (CardIdx_t)
-          hw_offset >> (G1CardTable::card_shift - LogHeapWordSize);
-
-      assert((size_t)from_card < HeapRegion::CardsPerRegion,
-             "Must be in range.");
+      CardIdx_t from_card = OtherRegionsTable::card_within_region(from, loc_hr);
       add_card_work(from_card, par);
     }
   }
@@ -343,10 +334,18 @@
          "just checking");
 }
 
+CardIdx_t OtherRegionsTable::card_within_region(OopOrNarrowOopStar within_region, HeapRegion* hr) {
+  assert(hr->is_in_reserved(within_region),
+         "HeapWord " PTR_FORMAT " is outside of region %u [" PTR_FORMAT ", " PTR_FORMAT ")",
+         p2i(within_region), hr->hrm_index(), p2i(hr->bottom()), p2i(hr->end()));
+  CardIdx_t result = (CardIdx_t)(pointer_delta((HeapWord*)within_region, hr->bottom()) >> (CardTable::card_shift - LogHeapWordSize));
+  return result;
+}
+
 void OtherRegionsTable::add_reference(OopOrNarrowOopStar from, uint tid) {
   uint cur_hrm_ind = _hr->hrm_index();
 
-  int from_card = (int)(uintptr_t(from) >> G1CardTable::card_shift);
+  uintptr_t from_card = uintptr_t(from) >> CardTable::card_shift;
 
   if (G1FromCardCache::contains_or_replace(tid, cur_hrm_ind, from_card)) {
     assert(contains_reference(from), "We just found " PTR_FORMAT " in the FromCardCache", p2i(from));
@@ -372,12 +371,8 @@
     prt = find_region_table(ind, from_hr);
     if (prt == NULL) {
 
-      uintptr_t from_hr_bot_card_index =
-        uintptr_t(from_hr->bottom())
-          >> G1CardTable::card_shift;
-      CardIdx_t card_index = from_card - from_hr_bot_card_index;
-      assert((size_t)card_index < HeapRegion::CardsPerRegion,
-             "Must be in range.");
+      CardIdx_t card_index = card_within_region(from, from_hr);
+
       if (G1HRRSUseSparseTable &&
           _sparse_table.add_card(from_hrm_ind, card_index)) {
         assert(contains_reference_locked(from), "We just added " PTR_FORMAT " to the Sparse table", p2i(from));
@@ -607,19 +602,12 @@
   if (_coarse_map.at(hr_ind)) return true;
 
   PerRegionTable* prt = find_region_table(hr_ind & _mod_max_fine_entries_mask,
-                                     hr);
+                                          hr);
   if (prt != NULL) {
     return prt->contains_reference(from);
 
   } else {
-    uintptr_t from_card =
-      (uintptr_t(from) >> G1CardTable::card_shift);
-    uintptr_t hr_bot_card_index =
-      uintptr_t(hr->bottom()) >> G1CardTable::card_shift;
-    assert(from_card >= hr_bot_card_index, "Inv");
-    CardIdx_t card_index = from_card - hr_bot_card_index;
-    assert((size_t)card_index < HeapRegion::CardsPerRegion,
-           "Must be in range.");
+    CardIdx_t card_index = card_within_region(from, hr);
     return _sparse_table.contains_card(hr_ind, card_index);
   }
 }
--- a/src/hotspot/share/gc/g1/heapRegionRemSet.hpp	Mon Mar 26 16:51:43 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionRemSet.hpp	Mon Mar 26 16:51:43 2018 +0200
@@ -130,6 +130,9 @@
   // be used to ensure consistency.
   OtherRegionsTable(HeapRegion* hr, Mutex* m);
 
+  // Returns the card index of the given within_region pointer relative to the bottom
+  // of the given heap region.
+  static CardIdx_t card_within_region(OopOrNarrowOopStar within_region, HeapRegion* hr);
   // Adds the reference from "from to this remembered set.
   void add_reference(OopOrNarrowOopStar from, uint tid);
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/gc/g1/TestFromCardCacheIndex.java	Mon Mar 26 16:51:43 2018 +0200
@@ -0,0 +1,120 @@
+/*
+ * @test TestFromCardCacheIndex.java
+ * @bug 8196485
+ * @summary Ensure that G1 does not miss a remembered set entry due to from card cache default value indices.
+ * @key gc
+ * @requires vm.gc.G1
+ * @requires vm.debug
+ * @requires vm.bits != "32"
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. -Xms20M -Xmx20M -XX:+UseCompressedOops -XX:G1HeapRegionSize=1M -XX:HeapBaseMinAddress=2199011721216 -XX:+UseG1GC -verbose:gc TestFromCardCacheIndex
+ */
+
+import sun.hotspot.WhiteBox;
+
+/**
+ * Repeatedly tries to generate references from objects that contained a card with the same index
+ * of the from card cache default value.
+ */
+public class TestFromCardCacheIndex {
+    private static WhiteBox WB;
+
+    // Shift value to calculate card indices from addresses.
+    private static final int CardSizeShift = 9;
+
+    /**
+     * Returns the last address on the heap within the object.
+     *
+     * @param The Object array to get the last address from.
+     */
+    private static long getObjectLastAddress(Object[] o) {
+        return WB.getObjectAddress(o) + WB.getObjectSize(o) - 1;
+    }
+
+    /**
+     * Returns the (truncated) 32 bit card index for the given address.
+     *
+     * @param The address to get the 32 bit card index from.
+     */
+    private static int getCardIndex32bit(long address) {
+        return (int)(address >> CardSizeShift);
+    }
+
+    // The source arrays that are placed on the heap in old gen.
+    private static int numArrays = 7000;
+    private static int arraySize = 508;
+    // Size of a humongous byte array, a bit less than a 1M region. This makes sure
+    // that we always create a cross-region reference when referencing it.
+    private static int byteArraySize = 1024*1023;
+
+    public static void main(String[] args) {
+        WB = sun.hotspot.WhiteBox.getWhiteBox();
+        for (int i = 0; i < 5; i++) {
+          runTest();
+          WB.fullGC();
+        }
+    }
+
+    public static void runTest() {
+        System.out.println("Starting test");
+
+        // Spray the heap with random object arrays in the hope that we get one
+        // at the proper place.
+        Object[][] arrays = new Object[numArrays][];
+        for (int i = 0; i < numArrays; i++) {
+            arrays[i] = new Object[arraySize];
+        }
+
+        // Make sure that everything is in old gen.
+        WB.fullGC();
+
+        // Find if we got an allocation at the right spot.
+        Object[] arrayWithCardMinus1 = findArray(arrays);
+
+        if (arrayWithCardMinus1 == null) {
+            System.out.println("Array with card -1 not found. Trying again.");
+            return;
+        } else {
+            System.out.println("Array with card -1 found.");
+        }
+
+        System.out.println("Modifying the last card in the array with a new object in a different region...");
+        // Create a target object that is guaranteed to be in a different region.
+        byte[] target = new byte[byteArraySize];
+
+        // Modify the last entry of the object we found.
+        arrayWithCardMinus1[arraySize - 1] = target;
+
+        target = null;
+        // Make sure that the dirty cards are flushed by doing a GC.
+        System.out.println("Doing a GC.");
+        WB.youngGC();
+
+        System.out.println("The crash didn't reproduce. Trying again.");
+    }
+
+    /**
+     * Finds an returns an array that contains a (32 bit truncated) card with value -1.
+     */
+    private static Object[] findArray(Object[][] arrays) {
+        for (int i = 0; i < arrays.length; i++) {
+            Object[] target = arrays[i];
+            if (target == null) {
+                continue;
+            }
+            final long startAddress = WB.getObjectAddress(target);
+            final long lastAddress = getObjectLastAddress(target);
+            final int card = getCardIndex32bit(lastAddress);
+            if (card == -1) {
+                Object[] foundArray = target;
+                return foundArray;
+            }
+        }
+        return null;
+    }
+}
+