Request for review: 6761744 Hotspot crashes if process size limit is exceeded
Tao Mao
tao.mao at oracle.com
Tue Apr 23 23:55:43 UTC 2013
New webrev:
http://cr.openjdk.java.net/~tamao/6761744/webrev.03/
jtreg test passed.
See inline.
Thanks.
Tao
On 4/22/13 12:58 AM, Bengt Rutisson wrote:
>
> Hi Tao,
>
> On 4/19/13 7:27 PM, Tao Mao wrote:
>> 1. check overflow routine is wrapped up.
>
> I would prefer that the method
> CollectedHeap::add_to_total_reserved_and_check_overflow() did not
> return a bool. Since you do vm_exit_during_initialization() when you
> overflow there is no need to return different results. This would also
> simplify the code where you use it. No need to use if statements.
It's fine to remove the bool return type. Done.
>
> Also, I think you should remove the naming references to
> "total_reserved" and "reserved". This method is general enough to just
> add two size_t arguments together and check for overflow. Especially
> if it would take the message as a parameter. Instead of returning a
> bool you can return the result of the addition if there was no overflow.
I'd like not to make the method generic for (1) it's not an appropriate
place to host such a generic add_to_and_check_overflow method; (2)
Remember, the refactoring was originally suggested to reduce duplication
of error message. Passing the error message gets back the flaw.
>
> I'm not sure we need round_up_total_reserved_and_check_overflow()
> since it is only being used in one place.
The same reason: avoid a duplication of string definition of error message.
>
>>
>> 2. The reduction of code duplication is adopted.
>
> Good.
>
>>
>> 3. Develop a way to intake 0~multiple external vm options in jtreg
>> main().
>
> Yes, this looks like it should work. However, based on John
> Cuthbertson's discussion with Leonid I think you need to do the same
> thing for test.java.opts too.
Followed the Leonid's latest suggestion.
>
> Bengt
>
>
>
>> 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.
>>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130423/4d86a87e/attachment.htm>
More information about the hotspot-gc-dev
mailing list