RFR: 8059066: CardTableModRefBS might commit the same page twice

[Erik Helin]

Hi all,

this patch fixes an issue with overlapping commits in cardTableModRefBS.
If the JVM is started with the options

   -Xmx8m -XX:-UseLargePages -version

on a system with a smallest page size of 8kB, then cardTableModRefBS 
will commit the same page twice. Given how os::commit_memory works, the 
issue is benign: mmap will simply overwrite the page with zeroes again, 
and since this is during initialization, the card table is only filled 
with zeroes. However, the issue becomes annoying when running with 
-XX:NativeMemoryTracking=detail, since NMT will report the overlapping 
commit.

The patch is a little tricky, since the surrounding code is a bit 
intertwined. I will try to explain the patch step-by-step, it is 
composed of two parts:
1. Update which committed regions we check when determining if
   `new_end_aligned` interferes with another region.
2. Update the criteria for checking if `new_end_aligned` interferes with
    another region.

The patch for the first part is:

--- a/src/share/vm/memory/cardTableModRefBS.cpp
+++ b/src/share/vm/memory/cardTableModRefBS.cpp
@@ -277,5 +277,5 @@ void CardTableModRefBS::resize_covered_r
      // space of another region.
      int ri = 0;
-    for (ri = 0; ri < _cur_covered_regions; ri++) {
+    for (ri = ind + 1; ri < _cur_covered_regions; ri++) {
        if (ri != ind) {
          if (_committed[ri].contains(new_end_aligned)) {

We only need to check if `new_end_aligned` clashes with regions 
following `ind`, since the code a few lines above handles regions before 
`ind`:

265     HeapWord* const max_prev_end = largest_prev_committed_end(ind);
266     if (max_prev_end > cur_committed.end()) {
267       cur_committed.set_end(max_prev_end);
268     }

We only commit more memory if
`new_end_for_commit > cur_committed.end()` where
`new_end_for_commit` is equal to new_end_for_aligned`. Hence, the case 
where a region before `ind` already have committed memory up to (or 
beyond) `new_end_aligned` is taken care of. Therefore, we only need to 
check regions after `ind`, which is the patch above. This is also 
confirmed by an existing comment and assert in the loop body:

285  // Any region containing the new end
286  // should start at or beyond the region found (ind)
287  // for the new end (committed regions are not expected to
288  // be proper subsets of other committed regions).
289  assert(_committed[ri].start() >= _committed[ind].start(),
290         "New end of committed region is inconsistent");

Since the committed regions should be sorted according their start 
address (see CardTableModRefBS::find_covering_region_by_base), the 
assert can only fail for regions where `ri < ind`.

The second part of the patch then updates the criteria for determining 
if `new_end_aligned` interferes with a region after `ind`:

--- a/src/share/vm/memory/cardTableModRefBS.cpp
+++ b/src/share/vm/memory/cardTableModRefBS.cpp
@@ -279,5 +279,6 @@ void CardTableModRefBS::resize_covered_r
      for (ri = ind + 1; ri < _cur_covered_regions; ri++) {
        if (ri != ind) {
-        if (_committed[ri].contains(new_end_aligned)) {
+        if (new_end_aligned > _committed[ri].start() &&
+            new_end_aligned <= _committed[ri].end()) {
            // The prior check included in the assert
            // (new_end_aligned >= _committed[ri].start())

`_committed[ri].contains(new_end_aligned)` only checks:

   bool contains(const void* addr) const {
     return addr >= (void*)_start && addr < (void*)end();
   }

and therefore misses the case when `new_end_aligned` completely covers 
the region _committed[ri], that is,
`new_end_aligned == _committed[ri].end()`. This is fixed by the above patch.

Given the above two patches, the `if (ri != ind`)` check can be removed 
from the loop body and the loop body can therefore remove two spaces of 
indentation. I also removed an outdated comment.

Webrev:
http://cr.openjdk.java.net/~ehelin/8059066/webrev.00/

Bug:
https://bugs.openjdk.java.net/browse/JDK-8059066

Testing:
- JPRT
- Ad-hoc (Window 32/64, Linux 32/64, OS X 64, Solaris x86-64/SPARC):
   - Dacapo, Kitchensink, Weblogic+medrec, runThese
   - nsk.regression, vm.gc, vm.oom, vm.parallel_class_loading,
     vm.regression, vm.runtime
   - nashorn regression tests
   - JTReg:
     - HotSpot
     - jdk_core, jdk_svc
- Running -Xmx8m -XX:-UseLargePages -version on a machine with 8kB pages

Thanks,
Erik