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

[sangheon]

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
>
> 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

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
>