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

[sangheon]

Thanks, Jon.

Sangheon


On 04/01/2016 02:16 PM, Jon Masamitsu wrote:
> Still looks good.
>
> Jon
>
>
> On 4/1/2016 8:26 AM, sangheon wrote:
>> Hi Tom,
>>
>> Thanks for reviewing this.
>>
>> On 04/01/2016 08:03 AM, Tom Benson wrote:
>>> Hi Sangheon,
>>> This looks fine to me except for a couple of trivial points in 
>>> commandLineFlagConstraintsGC.cpp comments.
>>>
>>> Typo in "should" :
>>> + // ParGCCardsPerStrideChunk shoule be
>> Fixed.
>>
>>> 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.
>> I like your suggestion and fixed.
>>
>> webrev:
>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.02
>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.02_to_01
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> 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/3865438a/attachment.htm>