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

[Stefan Karlsson]

Hi Bengt,

On 10/19/2011 11:45 AM, Bengt Rutisson wrote:
>
> Hi again,
>
> Updated webrev based on comments from Tony:
>
> http://cr.openjdk.java.net/~brutisso/7097516/webrev.02/

Looks good to me.

But, I find it really odd that Space::is_in_reserved(...) looks at the 
expanded size when working on Start Continues regions, and 
HeapRegion::is_in_reserved_raw looks at the "original" region size. To 
me, it would be more natural if Space::is_in_reserved(...) actually did 
what one would expect, look at the original heap region size, and then 
have a somthing like HeapRegion::is_in_reserved_expanded to expand the 
size for Start Continues regions.

StefanK

>
> Bengt
>
> On 2011-10-19 09:14, Bengt Rutisson wrote:
>>
>> 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
>