Request for review: 6761744 Hotspot crashes if process size limit is exceeded
Tao Mao
tao.mao at oracle.com
Tue Apr 30 21:52:48 UTC 2013
IMHO, what the customer reported could be a similar but different bug.
What version did they use, and what gc?
Thanks.
Tao
On 4/30/13 1:22 PM, Vladimir Kempik wrote:
> Hi
>
> It has easy reproducer.
>
> In solaris sparc, run 32-bit java
>
> java -Xmx4092M -version
>
> Java6/7 crashes. With patch from webrev.03 it works as it should,
> saying it has not enough memory .
> Vladimir
>
> 30.04.2013, в 22:11, Tao Mao <tao.mao at oracle.com
> <mailto:tao.mao at oracle.com>> написал(а):
>
>> 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/2103e8ea/attachment.htm>
More information about the hotspot-gc-dev
mailing list