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

[Jon Masamitsu]

On 04/07/2016 11:22 AM, sangheon wrote:
> 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

Looks good.  Thanks. for the changes.

Jon
>
> 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/69b2212b/attachment.htm>