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

[sangheon]

Hi Jon,

On 04/07/2016 10:36 AM, Jon Masamitsu wrote:
> 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"
Fixed.

>
> 492 value, sizeof(HeapWord)); From 
> share/vm/utilities/globalDefinitions.hpp you could use HeapWorkSize 
> const int HeapWordSize = sizeof(HeapWord);
I just repeated to use where it was calculated but I like is better.

Updated webrev:
http://cr.openjdk.java.net/~sangheki/8152182/webrev.03
http://cr.openjdk.java.net/~sangheki/8152182/webrev.03_to_02

Thanks,
Sangheon


> 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/a41c8da2/attachment.htm>