RFR(s): 8152182: Possible overflow in initialzation of _rescan_task_size and _marking_task_size

[Jon Masamitsu]

Sangheon,

Thanks for the changes. They look good.  A couple
of minor suggestions below.

Jon

On 04/06/2016 09:29 PM, sangheon wrote:
> Hi Jon,
>
> Thank you for the review.
>
> On 04/05/2016 05:02 PM, Jon Masamitsu wrote:
>> Sangheon,
>>
>> It occurs to me that if you created a function in CMS code 
>> check_rescan_task_size_alignment()
>> to do the check
>>
>>> 506 const size_t rescan_task_size = 
>>> cms->cmsSpace()->rescan_task_size();
>>> 507 const size_t alignment = CardTableModRefBS::card_size * 
>>> BitsPerWord;
>>> 508
>>> 509 if ((size_t)round_to((intptr_t)rescan_task_size, alignment) != 
>>> rescan_task_size) {
>>
>> then you won't have to expose the particular alignment requirements 
>> in the
>> constraint function.  The code could look more like const int 
>> HeapWordSize        = sizeof(HeapWord);
>>
>>
>> if ( check_rescan_task_size_alignment() ) {
>>
>> 513 CommandLineError::print(verbose,
>> 514 "Rescan task size (" SIZE_FORMAT " = CMSRescanMultiple * "
>> 515 "CardTableModRefBS::card_size_in_words * BitsPerWord) must be "
>> 516 "aligned to CardTableModRefBS::card_size * BitsPerWord (" 
>> SIZE_FORMAT "). "
>> 517 "Round-down value for CMSRescanMultiple is " SIZE_FORMAT "\n",
>> 518 rescan_task_size, alignment, round_down_value);
>> 519 status = Flag::VIOLATES_CONSTRAINT;
>>
>> }
>>
>>
>> You might be able to do something similar in the other constraint 
>> function.
> Your suggestion especially not exposing the alignment inspired me to 
> make it simpler. :)
> Task size is 'flag *CardTableModRefBS::card_size_in_words * 
> BitsPerWord' and we have to check the alignment with 
> CardTableModRefBS::card_size * BitsPerWord. As 
> CardTableModRefBS::card_size_in_words = CardTableModRefBS::card_size / 
> sizeof(HeapWord), if these flags are a multiple of sizeof(HeapWord), 
> the task size will be aligned.
>
>>
>> Also for
>>
>> 472 if (value > addr_ergo_max) {
>> 473 CommandLineError::print(verbose,
>> 474 "%s (" SIZE_FORMAT ") must be "
>> 475 "less than or equal to ergonomic maximum (" SIZE_FORMAT ") "
>> 476 "based on start address corresponds to the old generation of the 
>> Java heap\n",
>> 477 name, value, addr_ergo_max);
>> 478 return Flag::VIOLATES_CONSTRAINT;
>> 479 }
>>
>> Printing the addr_ergo_max might be confusing to the  users since it 
>> will be a very large
>> number.  Not sure what  to do unless you can print a maximum value of 
>> the flag based
>> on the maximum heap size (instead of based on MAX_SIZE).
> I agree that using max heap will be better, so I fixed.
> Considering your comment above (not exposing alignment information as 
> it is not nice), I had to add a new method to get ergonomic maximum 
> values for these task sizes. We can check their constraint without 
> this function, but can't print out ergo max. Or have to use 
> CardTableModRefBS::card_size_in_words * BitsPerWord on its calculation.
>
> Webrev:
> http://cr.openjdk.java.net/~sangheki/8152182/webrev.02
> http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01
http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html

469 "based on reserved area corresponds to the old generation of the 
Java heap\n",
"reserved area" is correct but perhaps not understandable for the 
average users. Maybe something like "which is based on the maximum size 
of the old generation of the Java heap\n"


492 value, sizeof(HeapWord)); From 
share/vm/utilities/globalDefinitions.hpp you could use HeapWorkSize 
const int HeapWordSize = sizeof(HeapWord); Jon

>
> Testing: JPRT, runtime/commandline JTREG on all platforms.
>
> Thanks,
> Sangheon
>
>
>>
>> Ask if that's not clear.
>>
>> Jon
>>
>>
>>
>> On 4/5/2016 10:24 AM, sangheon wrote:
>>> Hi all,
>>>
>>> Please review this change for CMSRescanMultiple and 
>>> CMSConcMarkMultiple flags.
>>>
>>> Both flags are set by "CardTableModRefBS::card_size_in_words * 
>>> BitsPerWord * flag" which potentially would make an overflow with 
>>> their maximum value without setting range. And these flags also 
>>> would make an arithmetic overflow when calculating with the size and 
>>> the start address of reserved area. In addition, CMSRescanMultiple 
>>> needs an alignment check.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152182
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8152182/webrev.00
>>> Testing: JPRT, runtime/commandline JTREG for all platforms
>>>
>>> Thanks,
>>> Sangheon
>>
>

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