Request for review: 6761744 Hotspot crashes if process size limit is exceeded
Tao Mao
tao.mao at oracle.com
Wed Apr 24 18:35:00 UTC 2013
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/20130424/79a3bca3/attachment.htm>
More information about the hotspot-gc-dev
mailing list