Request for review: 6761744 Hotspot crashes if process size limit is exceeded

[Tao Mao]

This should be handled by a separate CR (probably by this CR 
https://jbs.oracle.com/bugs/browse/JDK-7112912)

Thanks.
Tao

On 4/22/13 2:46 AM, Leonid Mesnik wrote:
> Tao
>
> This is not review, just question.
>
> Should you test pass on hosts with 512M-1GB memory and no swap?
>
> Leonid
> On 04/19/2013 09:27 PM, Tao Mao wrote:
>> 1. check overflow routine is wrapped up.
>>
>> 2. The reduction of code duplication is adopted.
>>
>> 3. Develop a way to intake 0~multiple external vm options in jtreg 
>> main(). Hope it helps others implement similar functionality.
>>
>> webrev:
>> http://cr.openjdk.java.net/~tamao/6761744/webrev.02/
>>
>> test:
>> JTREG: passed.
>> jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao 
>> <GC_OPTION>" 
>> /home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java
>>
>> where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC 
>> -XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC
>>
>>
>> Thanks.
>> Tao
>>
>> On 4/15/13 1:37 AM, Bengt Rutisson wrote:
>>>
>>> Hi Tao,
>>>
>>> I agree with John. The changes look fine and I'm ok with them. But 
>>> it would look nicer to add the 
>>> CollectedHeap::add_and_verify_no_overflow() method.
>>>
>>> Also, regarding the test, a couple of minor things.
>>>
>>> First, with the new naming convention I think the test should be 
>>> called something like:
>>>
>>> test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>>
>>> Then I think you can reduce the code duplication a bit. You create 
>>> the same ProcessBuilder and OutputAnalyzer in both 32 and 64 bit 
>>> cases. So maybe it could look like this instead:
>>>
>>> public class TestHandleExceedingProcessSizeLimitOn32BitSystems {
>>>   public static void main(String args[]) throws Exception {
>>>     ProcessBuilder pb =
>>>       
>>> ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),
>>>                                               "-Xmx3072m",
>>>                                               "-XX:MaxPermSize=1024m",
>>>                                               "-version");
>>>     OutputAnalyzer output = new OutputAnalyzer(pb.start());
>>>
>>>     String dataModel = System.getProperty("sun.arch.data.model");
>>>     if (dataModel.equals("32")) {
>>>       output.shouldContain("The size of the object heap + perm gen 
>>> exceeds the maximum representable size");
>>>       if (output.getExitValue() == 0) {
>>>         throw new RuntimeException("Not expected to get exit value 0");
>>>       }
>>>     } else if (dataModel.equals("64")) {
>>>       output.shouldHaveExitValue(0);
>>>     }
>>>   }
>>> }
>>>
>>> There is also a small bug in the test. If you run JTreg without 
>>> passing any vmoptions the test will fail. The reason is that 
>>> System.getProperty("test.vm.opts") will evaluate to the empty 
>>> string. This will be passed as the first argument to the Java 
>>> process, which will assume that this is the class name. So, it fails 
>>> to start because it can't load the class <empty string>.
>>>
>>> To reproduce the problem use this command line:
>>>
>>> java  -jar <path_to_jtreg>/lib/jtreg.jar 
>>> test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>>
>>> I guess the fix is to check that System.getProperty("test.vm.opts") 
>>> is not empty before passing it on to createJavaProcessBuilder().
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 4/13/13 3:15 AM, John Cuthbertson wrote:
>>>> Hi Tao,
>>>>
>>>> This looks OK to me.
>>>>
>>>> I can see what Thomas is saying though. All of the checks involve 
>>>> something like:
>>>>
>>>> total += some_value;
>>>> if (total < some_value) {
>>>>   // We must have overflowed
>>>>   vm_exit(...);
>>>> }
>>>>
>>>> So a function in CollectedHeap (the base class of SharedHeap and 
>>>> ParallelScavengeHeap) might make sense. For example:
>>>>
>>>> total_reserved = 0;
>>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>>> max_heap_size);
>>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>>> max_perm_size);
>>>>
>>>> Where the function is:
>>>>
>>>> size_t add_and_verify_no_overflow(size_t x, size_t y) {
>>>>   const char* msg = "...";
>>>>   x += y;
>>>>   if (x < y) {
>>>>     vm_exit(msg);
>>>>   }
>>>>   return x;
>>>> }
>>>>
>>>> But I can live with your current changes.
>>>>
>>>> JohnC
>>>>
>>>> On 4/10/2013 9:52 AM, Tao Mao wrote:
>>>>> Hi Bengt,
>>>>>
>>>>> Thank you for reviewing. A new webrev is updated.
>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.01/
>>>>>
>>>>> Cheers,
>>>>> Tao
>>>>>
>>>>> On 4/10/13 1:54 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi Tao,
>>>>>>
>>>>>> This change looks good. Thanks for adding the JTReg test, it 
>>>>>> looks good!
>>>>>>
>>>>>> One minor nit:
>>>>>>
>>>>>> In src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp I would 
>>>>>> suggest to store "(size_t) align_size_up(pgs->max_size(), 
>>>>>> HeapRegion::GrainBytes)" in a local variable rather than 
>>>>>> duplicating the calculation.
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>>
>>>>>> On 4/8/13 7:22 AM, Tao Mao wrote:
>>>>>>> 6761744 Hotspot crashes if process size limit is exceeded
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6761744
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.00/
>>>>>>>
>>>>>>> changeset:
>>>>>>> The fix only needs to go to hsx24 since there's no perm gen in 
>>>>>>> hotspot-25. Thus, the webrev is based on hsx24 repo.
>>>>>>>
>>>>>>> It provides for 32-bit builds a preventive check of the size of 
>>>>>>> "the object heap + perm gen" before reserving VM memory. The 
>>>>>>> total size should not exceed 4096MB (i.e. 4GB) for 32-bit 
>>>>>>> builds; otherwise, the total doesn't make sense and, what's 
>>>>>>> worse, overflow occurs. It will consequentially trigger anther 
>>>>>>> error of memory access violation, which was not protected.
>>>>>>>
>>>>>>> jtreg testing java code is also written, checking both 32-bit 
>>>>>>> and (trivially) 64-bit builds.
>>>>>>>
>>>>>>> testing:
>>>>>>> check jtreg tests with flags -XX:+UseParallelGC, -XX:+UseG1GC, 
>>>>>>> -XX:+UseParNewGC, -XX:+UseConcMarkSweepGC, -XX:+UseSerialGC and 
>>>>>>> builds of 32-bit and 64-bit. All passed.
>>>>>>>
>>>>>>> Needs JPRT test when pushing.
>>>>>>
>>>>
>>>
>
>
> -- 
> Leonid Mesnik
> JVM SQE
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130422/d4f313b9/attachment.htm>