Request for review (S): 7097516 G1: assert(0<= from_card && from_card<HeapRegion::CardsPerRegion) failed: Must be in range.

[Bengt Rutisson]

Hi all,

Could I have a couple of reviews of this change?

http://cr.openjdk.java.net/~brutisso/7097516/webrev.01/

Some background:

The humongous start regions in G1 gets their _end marker set to the end 
of the last humongous continues region for the humongous object. This 
means that the start region overlaps all of the continues regions. Code 
that handles regions need to be aware of this special case.

However, PerRegionTable::add_reference_work() had a subtle issue with 
this special case. Here is what happened:

A thread, TA, looks up the PerRegionTable, PRT,  for a particular heap 
region. This heap region happens to be a humongous continues region. 
Let's call it HRA.

Another thread, TB, wants to look up the PerRegionTable for the 
humongous start region of that same humongous object. Let's call that 
heap region HRB. If we are unlucky TB is the first thread that wants the 
PerRegionTable for HRB and thus has to allocate it. Now, there is a 
limit to how many PerRegionTables we allow in the system. If this limit 
has been reached TB will find a suitable existing PerRegionTable and 
coarsen that to use a bitmap for the remset instead of the PerRegionTable.

This means that TB can actually "steal" the PRT from underneath TA.

TA is aware that this can happen. So, before it tries to do any work 
with the information in the PRT it tries to verify that it hasn't been 
"stolen" (actually coarsened). It does this by storing the heap region 
for the PRT in a local variable (called loc_hr) and then check that the 
reference that it is about to handle still belong to the heap region.

Basically:

     HeapRegion* loc_hr = hr();
     if (loc_hr->is_in_reserved(from)) {
        ...
     }

Normally this is safe. If TA gets HRB in loc_hr the test will fail. 
However, in the case where HRB is the humongous start region the test 
will not fail. Instead we will pass the test and execute the code inside 
the if. There we try to calculate a card index based on the bottom of 
the heap region and the from address. Now this card index will be larger 
than the number of cards we have per region, since from actually is not 
in the region.

The fix is to introduce a new method called 
HeapRegion::is_in_reserved_raw() and use this in the test. The _raw 
method will not return false even for humongous start objects in the 
example above.

I picked the name is_in_reserved_raw() to be consistent with the 
heap_region_containing()/heap_region_containing_raw() methods. I am not 
particularly fond of the name, so if there are better suggestions I am 
happy to change it.

Testing
I was able to reproduce the issue on bur398-216 after 40-50 iterations. 
With my fix it has now ran 540 iteration without failing.

Other uses of is_in_reserved()
Tony and I went through the G1 code and looked for other uses of 
HeapRegion::is_in_reserved(). As far as we can tell the other uses all 
avoid the issue that PerRegionTable::add_reference_work() has. Most of 
them because they are used during evacuation where humongous objects are 
not present at all.

Webrev:
http://cr.openjdk.java.net/~brutisso/7097516/webrev.01/

CR:
7097516 G1: assert(0<= from_card && 
from_card<HeapRegion::CardsPerRegion) failed: Must be in range.
http://monaco.us.oracle.com/detail.jsf?cr=7097516

Thanks,
Bengt