RFR(s): 8152176: Big ParGCCardsPerStrideChunk values can cause overflow for CMS GC

[Tom Benson]

Hi Sangheon,
This looks fine to me except for a couple of trivial points in 
commandLineFlagConstraintsGC.cpp comments.

Typo in "should" :

+ // ParGCCardsPerStrideChunk shoule be

I think the "However," in this line is not needed, and confusing:

+ // from CardTableModRefBSForCTRS::process_stride(). However, 
ParGCStridesPerThread is already checked

I'd probably say "Note that ParGCStridesPerThread ..." instead.

Tom


On 3/29/2016 6:42 PM, sangheon wrote:
> Thanks, Jon.
>
> Sangheon
>
>
> On 03/29/2016 02:13 PM, Jon Masamitsu wrote:
>>
>>
>> On 3/29/2016 12:07 PM, sangheon wrote:
>>> Hi Jon,
>>>
>>> Thank you for reviewing this.
>>>
>>> On 03/29/2016 10:49 AM, Jon Masamitsu wrote:
>>>> Sangheon,
>>>>
>>>> I agree that we don't want to expose 
>>>> CardTableModRefBS::_last_valid_index
>>>> but I'd consider making CardTableModRefBS::cards_required() public.
>>>> That would remove the need to duplicate the calculation and it seems
>>>> a reasonable question to ask the card table implementation how many
>>>> cards it is going to require.  I'll let other disagree if they like.
>>> Good idea and changed to 'public'.
>>>
>>>>
>>>> Add in the message a little more about why the value is too large.
>>>>
>>>>
>>>> 396 CommandLineError::print(verbose,
>>>> 397 "ParGCCardsPerStrideChunk (" INTX_FORMAT ") "is too large for 
>>>> the heap size and " "must be "
>>>> 398 "less than or equal to card table size (" SIZE_FORMAT ")\n",
>>>> 399 value, card_table_size);
>>> Fixed.
>>>
>>>> I think a little more information here would help future maintainers so
>>>> instead of
>>>> 403 // If n_strides which is used with ParGCCardsPerStrideChunk is 
>>>> really large, we would face an overflow.
>>>>
>>> I updated the comment.
>>>
>>>> This statement can overflow?
>>>>
>>>> 404 uintx n_strides = ParallelGCThreads * ParGCStridesPerThread;
>>> No.
>>> Because above statement is checked not to overflow from 
>>> ParGCStridesPerThreadConstraintFunc() which is called before this 
>>> constraint function. (added this explanation as well)
>>
>> Ok.
>>
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.01/
>>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.01_to_00/
>>
>> Looks good.
>>
>> Jon
>>
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>
>>>> Jon
>>>>
>>>> On 3/28/2016 3:50 PM, sangheon wrote:
>>>>> Hi all,
>>>>>
>>>>> Could I have some reviews for this change?
>>>>>
>>>>> As very large value of ParGCCardsPerStrideChunk flag would result 
>>>>> in an overflow, we need a constraint function after memory 
>>>>> initialization. And the function will check the flag to be less 
>>>>> than of equal to the size of card table and not to make an 
>>>>> overflow with other stride factors(ParallelGCThreads and 
>>>>> ParGCStridesPerThread).
>>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152176
>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8152176/webrev.00
>>>>> Testing: JPRT, all runtime/commandline JTREG tests on all platforms
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160401/86bdbcc2/attachment.htm>