Round 2 Code Review request for new MallocMaxBytes option and fixed test (7030610, 7037122, 7123945)

[Ron Durbin]

Ioi,

Thanks for the review!


On 3/18/13 12:38 PM, Ioi Lam wrote:
> Hi Ron,
>
> Is it possible to change MallocMaxTestWords back MallocMaxTestBytes?
> The reasons are:
>
>  * I am surprised that Coleen hasn't complained yet.

No, Coleen requested the change from MallocMaxTestBytes to MallocMaxTestWords.


>  * It would be good if you can say -XX:MallocMaxTestBytes=128M and
>    won't be confused about what it means.

Yes, the 'M' suffix (and the others) are useful shorthands, but we
switched to "test words" to avoid having to use jlongs in some of
the calculations.


>  * For the tests that need to use this option (for testing the handling
>    of malloc failures), do they really need to allocate more than 2GB
>    of stuff?

In reality, we're likely to use this option to restrict tests to something
less than 2GB, but that's our intended usage. Why restrict it if someone
want to use a larger value? By using Words, we have a greater range of
possible values; see the other GC options that specify sizes in Words.


>  * (alloc_size / BytesPerWord) will give you zero if the test case keep
>    allocating 7 bytes or less (on 64-bit).

Allocation values this small aren't really in the use case that we're
planning for this test option. We're looking for a rough like this:

     if you've allocated more than 3/4 GB, then fail

It could be a little more, it could be a little less. Something in
the ballpark.


> for overflow checks, you probably need
>
>    if (int(old_bytes + new_bytes) < (int)(old_bytes) ||
>         old_bytes + new_bytes > MallocMaxTestWords) {
>         return NULL; // fail to alloc
>    }

There is an existing "roll over" style check in os::malloc() so this
one isn't necessary.


> I suggest limiting MallocMaxTestWords to no more than 0x7fffffff so 
> that the logic would be a little cleaner and easier to understood.

I doubt that you'd every be able to get anywhere close to that
number on a real system, but again, why limit it?


> Maybe we should also add sizeof(int*) to the requested size, and round 
> the size up to sizeof(int*)? This would be a good estimate for the 
> malloc overhead.

This option isn't intended to be this accurate. See the reply above.

Ron


>
> - Ioi
>
>
> On 03/15/2013 11:29 AM, Ron Durbin wrote:
>>
>> Dan
>>
>> I have a round 2 code review ready for:
>>
>> Round 2 addresses:
>>
>> - addressed code review comments from Coleen, Dan and David
>> - note: src/share/vm/runtime/os.hpp is no longer changed.
>>
>> Web URL 
>> http://cr.openjdk.java.net/~rdurbin/7030610-webrev-cr1/1-hsx25/ 
>> <http://cr.openjdk.java.net/%7Erdurbin/7030610-webrev-cr1/1-hsx25/>
>>
>> Internal URL 
>> http://javaweb.us.oracle.com/~rdurbin/7030610-webrev/1-hsx25/ 
>> <http://javaweb.us.oracle.com/%7Erdurbin/7030610-webrev/1-hsx25/>
>>
>> 7030610 runtime/6878713/Test6878713.sh fails, Error. failed to clean 
>> up files after test.
>>
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7030610
>>
>> https://jbs.oracle.com/bugs/browse/JDK-7030610
>>
>> 7037122 runtime/6878713/Test6878713.sh is written incorrectly
>>
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7037122
>>
>> https://jbs.oracle.com/bugs/browse/JDK-7037122
>>
>> (This fix only addresses the issues with test 
>> runtime/6878713/Test6878713.sh;
>>
>> 7037122 names other tests that are not addressed by this fix)
>>
>> 7123945 runtime/6878713/Test6878713.sh require about 2G of
>>
>> _http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123945_
>>
>> https://jbs.oracle.com/bugs/browse/JDK-7123945
>>
>> Summary: Add new diagnostic option -XX:MallocMaxTestWords=NNN and fix
>>
>> Test6878713.sh. Test6878713.sh is a security regression test with 
>> four different
>> failure modes.Three are documented in the defects above and the forth
>>
>> was found during bug triage. The root cause of these failures can be
>>
>> traced back to two issues: Inconsistent test condition setup and 
>> error handling.
>>
>> All four defect manifestations are fixed by this set of changes.
>>
>> The testing is not completed because the JPRT run is running slowly.
>>
>
>