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

[Tony Printezis]

Stefan,

I discussed this with Bengt earlier this week.

When we allocate a humongous object and we set up a series of humongous 
regions to contain it (one starts humongous - call it SH - and zero or 
more continues humongous - call them CH), we extend the SH region to 
cover the entire series. This is not only so that is_in_reserved() 
checks that the address is contained in the entire series but also that 
capacity(), used(), etc. return a result that corresponds to the entire 
series (and all those methods need to be consistent with each other). If 
we didn't do this, we'd have to produce special methods for all of them 
and, when we'd like to get some info about the region, we'd have to 
first check whether it's SH or not and call the appropriate method.

Note, however, that the only time we really need to "original" version 
of the above methods is in the current RSet code given that each RSet 
"chunk" corresponds to exactly one region. Everything else in the code 
relies on the fact that the above methods on SH regions are the 
"extended" versions. So, we save ourselves a lot of trouble but not 
having to check whether a region is SH or not before querying it.

I'd also like to point out that I'd like us to re-architect the RSets so 
that they do not rely on regions but, instead, on (fixed size) memory 
ranges (could be equal to the region size, could be smaller, could be 
larger). That way, we'll eliminate the need for the "raw" version of 
is_in_reserved() and deal with SH and non-humongous regions uniformly.

Tony

On 10/20/2011 04:21 AM, Stefan Karlsson wrote:
> 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
>>
>