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

[Thomas Schatzl]

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