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

[Yasumasa Suenaga]

Thanks Thomas!

I uploaded new webrev:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.02/

But I guess we can remove the following change:

--------------
@@ -1477,6 +1483,9 @@
    // the HWM, an allocation is still attempted. This is because another thread must then
    // have incremented the HWM and therefore the allocation might still succeed.
    do {
+    if (MetaspaceGC::capacity_until_GC() + delta_bytes > MaxMetaspaceSize) {
+      return NULL;
+    }
      incremented = MetaspaceGC::inc_capacity_until_GC(delta_bytes, &after, &before);
      res = allocate(word_size, mdtype);
    } while (!incremented && res == NULL);
--------------

OTOH inc_capacity_until_GC() shouldn't be passed the value which exceeds MaxMetaspaceSize.
Should we keep this change?


Thanks,

Yasumasa



On 2019/01/30 21:55, Thomas Schatzl wrote:
> On Wed, 2019-01-30 at 21:49 +0900, Yasumasa Suenaga wrote:
>> Hi Thomas,
>>
>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.01/
>>>>
>>>> I write reply based on it as below.
>>
>>>> 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?
> 
> No, in MetaspaceGC::inc_capacity_until_GC() after the cmpxchg and the
> check whether this thread has been the one incrementing the watermark.
> 
> I.e. "
> 
> size_t prev_value = Atomic::cmpxchg(new_value, ...);
> 
> if (old_capacity_until_gc != prev_value) {
>    return false; // Somebody else incremented the counter
> }
> 
> assert(new_value <= MaxMetaspaceSize, "...
> 
> "
> 
> At that point in execution we are sure this thread set "new_value" to a
> new value, that should obviously not exceed MaxMetaspaceSize.
> 
> Please use a meaningful assert text containing the increment, new value
> and threshold.
> 
> Thanks,
>    Thomas
> 
>