Request for review: 6761744 Hotspot crashes if process size limit is exceeded
Leonid Mesnik
leonid.mesnik at oracle.com
Wed Apr 24 10:36:09 UTC 2013
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/7a11ead4/attachment.htm>
More information about the hotspot-gc-dev
mailing list