PING: RFR: 8217432: MetaspaceGC::_capacity_until_GC exceeds MaxMetaspaceSize

Yasumasa Suenaga yasuenag at gmail.com
Wed Jan 30 12:49:25 UTC 2019


Hi Thomas,

>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.01/
>>
>> I write reply based on it as below.
> 
>    internal testing shows no issues with the patch.

Thanks!


>> As I said before, I added assert() to inc_capacity_until_GC().
>> It will fail without other change in this webrev. So I think we can
>> evaluate it via
>> vmTestbase/metaspace/shrink_grow/ShrinkGrowTest/ShrinkGrowTest.java .
> 
> Okay, I guess that's enough. Please add a @bugid with this CR number to
> the test then.

I will add it.


>> I added the code to check total amount of _capacity_until_GC
>> before inc_capacity_until_GC() call.
>> But I concern the check and increment are not atomically. Is it okay?
> 
> If the check were located after the Atomic::cmpxchg() and after the
> check whether it had been this thread to change the value it would be
> fine :)

Do you suggest as following code?

------------------
@@ -1478,6 +1482,12 @@
    // have incremented the HWM and therefore the allocation might still succeed.
    do {
      incremented = MetaspaceGC::inc_capacity_until_GC(delta_bytes, &after, &before);
+    // Check after the increment that we did not go over the maximum.
+    // We can not do this earlier due to potential races.
+    if (!incremented && (after > MaxMetaspaceSize)) {
+      MetaspaceGC::dec_capacity_until_GC(delta_bytes);
+      return false;
+    }
      res = allocate(word_size, mdtype);
    } while (!incremented && res == NULL);

------------------

If so, assert() in inc_capacity_until_GC() might be failed
because _capacity_until_GC + delta_bytes might exceed MaxMetaspaceSize.

If we can change arguments of inc_capacity_until_GC(), we can pass
expected value to it (_capacity_until_GC + delta_bytes).
If so, we can compliant MaxMetaspaceSize via cmpxchg.


Thanks,

Yasumasa


On 2019/01/30 20:04, Thomas Schatzl wrote:
> Hi Yasumasa,
> 
> On Tue, 2019-01-29 at 21:39 +0900, Yasumasa Suenaga wrote:
>> Thank you for your comment.
>> I uploaded new webrev (it passed all tests on submit repo):
>>
>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.01/
>>
>> I write reply based on it as below.
> 
>    internal testing shows no issues with the patch.
> 
>> On 2019/01/28 22:55, Thomas Schatzl wrote:
>>> Hi Yasumasa,
>>>
>>> On Fri, 2019-01-25 at 11:35 +0900, Yasumasa Suenaga wrote:
>>>> PING: Could you review this?
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/01/21 17:52, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> I filed this issue to JBS:
>>>>>      https://bugs.openjdk.java.net/browse/JDK-8217432
>>>>>
>>>>> I investigated it, and I found that `minimum_desired_capacity`
>>>>> and `maximum_desired_capacity` in
>>>>> `MetaspaceGC::compute_new_size()`
>>>>> might exceed MaxMetaspaceSize.
>>>>> They shouldn't exceed MaxMetaspaceSize.
>>>>>
>>>>> I uploaded webrev for this bug. Could you review it?
>>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.00/
>>>>>
>>>>> This change has passed all tests on submit repo,
>>>>> :vmTestbase_vm_metaspace,
>>>>> and :vmTestbase_nsk_sysdict jtreg tests.
>>>
>>> The suggested change does not work - you can't return false from
>>> MetaspaceGC::inc_capacity_until_gc() because the return value
>>> indicates whether it was possible to change the value, not whether
>>> the value passed sense.
>>>
>>> The code before that must make sure that the resulting value is
>>> within limits (that could be a good assert in
>>> inc_capacity_until_GC()) from my understanding, without being an
>>> expert here.
>>>
>>> So with the current change, the assert in metaspace.cpp:260 would
>>> simply fire in this case.
>>
>> I added assert() to inc_capacity_until_GC() because
>> _capacity_until_GC shouldn't exceed MaxMetaspaceSize.
>> This function will not return false when _capacity_until_GC attempt
>> to be exceeded MaxMetaspaceSize.
>>
>>
>>> Making sure that the HWM does not go over MaxMetaspaceSize could be
>>> achieved by limiting expand_bytes passed to the method; I *think*,
>>> without following where minimum_desired_capacity is also used in
>>> that method, clamping it via
>>>
>>> 241:   minimum_desired_capacity =
>>> MIN2(MAX2(minimum_desired_capacity,
>>> MetaspaceSize), MaxMetaspaceSize);
>>>
>>> would achieve the desired effect.
>>
>> Your suggestion seems to be equivalent to my change.
> 
> Okay :)
> 
>> -------------
>>      const double min_tmp = used_after_gc / maximum_used_percentage;
>>      size_t minimum_desired_capacity =
>> -    (size_t)MIN2(min_tmp, double(max_uintx));
>> +    (size_t)MIN2(min_tmp, double(MaxMetaspaceSize));
>>      // Don't shrink less than the initial generation size
>>      minimum_desired_capacity = MAX2(minimum_desired_capacity,
>>                                      MetaspaceSize);
>> -------------
>>
>> I don't understand why current implementation use `max_uintx` at
>> this.
>> I guess it means "max value of Metaspace", so I think it should use
>> MaxMetaspaceSize rather than max_uintx.
> 
> I agree.
> 
>>> Could you also provide a test case for this change (i.e. failing
>>> before, good after) as Aleksey suggested?
>>
>> It is hard to reproduce this problem because this issue is caused by
>> C++ member variable (MetaspaceGC::_capacity_until_GC).
>>
>> It can be tested with WhiteBox test (incMetaspaceCapacityUntilGC()),
>> however it tests inc_capacity_until_GC() call only.
>>
>> OTOH we might test as Aleksey said, but we can't estimate precise
>> commit memory size of Metaspace, and evaluate why OOME is occurred.
>> (memory consumption or invalid _capacity_until_GC)
>>
>> As I said before, I added assert() to inc_capacity_until_GC().
>> It will fail without other change in this webrev. So I think we can
>> evaluate it via
>> vmTestbase/metaspace/shrink_grow/ShrinkGrowTest/ShrinkGrowTest.java .
> 
> Okay, I guess that's enough. Please add a @bugid with this CR number to
> the test then.
> 
>>> Note that my suggestion only fixes the call to
>>> inc_capacity_until_gc() in MetaspaceGC::compute_new_size(). I very
>>> much believe it is required to add another guard in
>>> ClassLoaderMetaspace::expand_and_allocate().
>>>
>>> (Please add the assert in inc_capacity_until_gc() I mentioned
>>> above!).
>>
>> I added the code to check total amount of _capacity_until_GC
>> before inc_capacity_until_GC() call.
>> But I concern the check and increment are not atomically. Is it okay?
> 
> If the check were located after the Atomic::cmpxchg() and after the
> check whether it had been this thread to change the value it would be
> fine :)
> 
> Please add a comment like "Check after the increment that we did not go
> over the maximum. We can not do this earlier due to potential races"
> before it to explain its location.
> 
> Thanks,
>    Thomas
> 
> 


More information about the hotspot-gc-dev mailing list