RFR: 8049599: MetaspaceGC::_capacity_until_GC can overflow

[Erik Helin]

On 2014-09-30 21:42, Jon Masamitsu wrote:
> Erik,
>
> Changes look good.
>
> Reviewed.

Thanks!
Erik

> Jon
>
> On 9/30/2014 5:43 AM, Erik Helin wrote:
>> All,
>>
>> got some great feedback from StefanK:
>> - Use cmpxchg_ptr instead of cmpxchg
>> - Add a comment describing the while loop in expand_and_allocate
>> - Add a comment in the test describing the overflow attempt
>> - Change the loop in expand_and_allocate to do/while
>> - Shorten the names of the local variables in expand_and_allocate
>>
>> The result can be seen in the following webrevs:
>> - full: http://cr.openjdk.java.net/~ehelin/8049599/webrev.02/
>> - inc: http://cr.openjdk.java.net/~ehelin/8049599/webrev.01-02/
>>
>> Thanks,
>> Erik
>>
>> On 2014-09-24 18:32, Erik Helin wrote:
>>> All,
>>>
>>> I've reworked the patch quite a bit based on (great!) internal feedback
>>> from StefanK and Mikael Gerdin. The patch still uses an overflow check
>>> and a CAS to update the high-water mark (HWM), but the new behavior
>>> should be the same as the old one (which used Atomic::add_ptr).
>>>
>>> With the current code, each thread always increments the HWM, but there
>>> is a race in that another thread can allocate metadata (due to the
>>> increased HWM) before the thread that increased the HWM gets around to
>>> allocate. With the new code, each thread will increase the pointer at
>>> most once using a CAS. Even if increasing the HWM fails, the allocation
>>> attempt might still succeed (for the reason described above).
>>>
>>> There is a theoretical problem of starvation in the new code, a thread
>>> might forever fail to increase the HWM and forever fail to allocate due
>>> to contention, but in practice this should not be a problem. In the
>>> current code, Atomic::add_ptr is implemented as a CAS in a
>>> (theoretically) never ending loop on non-x86 CPUs, so the same
>>> theoretical starvation problem is present in the current code as well
>>> (on non-x86 CPUs that is).
>>>
>>> Webrevs:
>>> - full:
>>>    http://cr.openjdk.java.net/~ehelin/8049599/webrev.01/
>>> - incremental:
>>>    http://cr.openjdk.java.net/~ehelin/8049599/webrev.00-01/
>>>
>>> Testing:
>>> - JPRT
>>> - Aurora:
>>>    - Kitchensink
>>>    - Weblogic+Medrec
>>>    - runThese
>>>    - vm.quick, regression, gc, compiler, runtime, parallel class
>>> loading,
>>>      metaspace, oom
>>>    - JTReg tests
>>> - Running newly added JTREG test
>>>
>>> Thanks,
>>> Erik
>>>
>>> On 2014-08-20 12:27, Erik Helin wrote:
>>>> Hi all,
>>>>
>>>> this patch fixes a problem where Metaspace::_capacityUntilGC can
>>>> overflow ("wrap around"). Since _capacityUntilGC is treated as a size_t
>>>> everywhere it used, we won't calculate with negative numbers, but an
>>>> eventual wrap around will still cause unnecessary GCs.
>>>>
>>>> The problem is solved by detecting an eventual wrap around in
>>>> Metaspace::incCapacityUntilGC. The overflow check means that
>>>> _capacityUntilGC now must be updated with a CAS. If the CAS fails more
>>>> than five times due to contention, no update will be done, because this
>>>> means that other threads must have incremented _capacityUntilGC (it is
>>>> decremented only during a safepoint). This also means that a thread
>>>> calling incCapacityUntilGC might have "its" requested memory
>>>> "stolen" by
>>>> another thread, but incCapacityUntilGC has never given any fairness
>>>> guarantees.
>>>>
>>>> The patch also adds two functions to the WhiteBox API to be able to
>>>> update and read Metaspace::_capacityUntilGC from a JTREG test.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8049599
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~ehelin/8049599/webrev.00/
>>>>
>>>> Testing:
>>>> - JPRT
>>>> - Aurora ad-hoc testing (on all platforms, both 32-bit and 64-bit):
>>>>    - Kitchensink, runThese and Dacapo
>>>>    - JTREG tests
>>>>    - Parallel Class Loading testlist
>>>>    - GC, runtime and compiler testlists
>>>>    - OOM and stress testlists
>>>> - Running newly added JTREG test
>>>>
>>>> Thanks,
>>>> Erik
>