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

[sangheon]

Thanks Jon and Jesper for the review.

Sangheon


On 04/07/2016 02:48 PM, Jesper Wilhelmsson wrote:
> Latest changes looks good to me as well.
> /Jesper
>
> Den 7/4/16 kl. 23:31, skrev 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
>>>>>>
>>>>>
>>>>
>>>
>>