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

Vladimir Kempik vladimir.kempik at oracle.com
Tue Apr 30 10:33:56 UTC 2013


Hello.

I have a customer with non-escalated bug that is duplicate of this bug. 
I checked with patch from webrev.03 and can confirm that fixed the bug.

Could you guys approve it if it's fine?

Thanks. Vladimir.
On 24.04.2013 22:35, Tao Mao wrote:
> For 32-bit builds: The current changeset provides the first 
> "protection" of heap size handling. Then comes handling whether we can 
> allocate a certain amount of a heap.
>
> For 64-bit builds: a machine with 512M-1GB memory should fail in many 
> other tests as well, due to inability to lauch jvm.
>
> Anyway, I ran the test and it passed.
>
> --------------------------------------------------
> Simply,
>
> -bash-4.1$ ulimit -v 134217728 (128m for human reading)
> -bash-4.1$ ulimit -m 134217728 (128m FHR)
>
> (cannot limit virtual memory to zero or set vm.swappiness=0, in which 
> case the server would hang immediately w/o even launching jvm. But, 
> the setting should satisfy your question.)
>
> Then, run jtreg. Then, pass.
> --------------------------------------------------
>
> BTW, since you've jumped in this thread ^_^ can you check the way I 
> get system property through "test.java.opts" to set inside jvm 
> process? It's in 
> test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java
>
> Thank you.
> Tao
>
> On 4/24/13 3:36 AM, Leonid Mesnik wrote:
>> On 04/22/2013 10:07 PM, Tao Mao wrote:
>>> This should be handled by a separate CR (probably by this CR 
>>> https://jbs.oracle.com/bugs/browse/JDK-7112912)
>>>
>> Why this? I mean should your test pass on hosts with *small* amount 
>> of memory.
>>
>> Leonid
>>> 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
>>
>>
>> -- 
>> Leonid Mesnik
>> JVM SQE

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130430/3b01191a/attachment.htm>


More information about the hotspot-gc-dev mailing list