Request for review: 6761744 Hotspot crashes if process size limit is exceeded
Tao Mao
tao.mao at oracle.com
Tue Apr 30 18:11:43 UTC 2013
Hi Vladimir,
Can you point me to your bug you think it's associated with?
Thanks.
Tao
On 4/30/13 3:33 AM, Vladimir Kempik wrote:
> 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/ffb14166/attachment.htm>
More information about the hotspot-gc-dev
mailing list