RFR (S) 8213092: Add more runtime locks for concurrent class unloading

Tue Oct 30 18:23:53 UTC 2018

On 10/30/18 1:47 PM, Doerr, Martin wrote:
> Hi Coleen,
>
> I had only taken a quick look at the change and this was the first concern I had. I just wanted to post it ASAP.
> I haven't understood "is_concurrent = SafepointSynchronize::is_at_safepoint()". Isn't there a "!" missing (systemDictionary.cpp)?

You are right.  I'm going to have to retest this.

Thank you!
Coleen

>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: coleen.phillimore at oracle.com <coleen.phillimore at oracle.com>
> Sent: Dienstag, 30. Oktober 2018 18:13
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR (S) 8213092: Add more runtime locks for concurrent class unloading
>
>
> Hi Martin,
>
> I see that now:
> find _enclosing_space is called by find_enclosing_virtual_space is
> called by is_range_in_committed is called by oopDesc::is_valid and
> Klass::is_valid which are both used for error reporting.
>
> I'm going to revert the metaspace.cpp and virtualSpaceList.cpp changes
> and let Erik find out why he wanted them :)
>
> Is the rest reviewed?
>
> Thanks!
> Coleen
>
> On 10/30/18 10:09 AM, Doerr, Martin wrote:
>> Hi Coleen,
>>
>> I think the acquisition of MetaspaceExpand_lock should get moved to VirtualSpaceList::contains because find_enclosing_space is used during error reporting. If the VM crashes while the lock is held, error reporting will hang.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net> On Behalf Of coleen.phillimore at oracle.com
>> Sent: Dienstag, 30. Oktober 2018 14:21
>> To: hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR (S) 8213092: Add more runtime locks for concurrent class unloading
>>
>>
>>
>> On 10/30/18 9:16 AM, coleen.phillimore at oracle.com wrote:
>>> On 10/30/18 8:36 AM, David Holmes wrote:
>>>> On 30/10/2018 10:24 PM, coleen.phillimore at oracle.com wrote:
>>>>> On 10/30/18 12:17 AM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 30/10/2018 1:45 PM, coleen.phillimore at oracle.com wrote:
>>>>>>> Summary: Add locks for calling CLDG::purge concurrently as well
>>>>>>> and for calling SystemDictionary::do_unloading concurrently.
>>>>>>>
>>>>>>> Ran linux-x64 tier1-6 through mach5 and hotspot/jtreg/runtime
>>>>>>> tests, which include the module tests.
>>>>>>>
>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8213092.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8213092
>>>>>> So ... all the locks covered by an assert_locked_or_safepoint, or
>>>>>> which are acquired by the VMThread at a safepoint, must never be
>>>>>> held by a JavaThread if it could reach a safepoint whilst that lock
>>>>>> is held - else we could deadlock. So can we check that with
>>>>>> NoSafepointVerifiers?
>>>>> Actually I think this is not possible to add NSV.   You can acquire
>>>>> the ClassLoaderDataGraph_lock and then the Module_lock. The latter
>>>>> would check for a safepoint also for a Java thread. This is
>>>>> currently done for Jvmti and JFR, but not in other code that I can
>>>>> see.  I don't actually know how to fix this problem.
>>>> This seems risky. If a JavaThread can hold the CLDG_lock while
>>>> blocked at a safepoint (acquiring the Module_lock), then what is to
>>>> stop the VMThread from hitting one of these sections of code
>>>> protected by locked_or_safepoint and then proceeding into what is
>>>> effectively a critical section (by virtue of there being a safepoint)
>>>> when the JavaThread is itself in the midst of a critical section? Do
>>>> we actively take steps to prevent this somehow, or to make it safe
>>>> for the VMThread to proceed?
>> I might not have answered your question about this lock in particular.
>> There is only the linking and unlinking (in a safepoint except at ZGC)
>> that are protected by CLDG_lock, and these are not interrupted by a
>> safepoint.  So this is safe.
>>
>> Coleen
>>> No we don't.  I think we have this problem today (not introduced or
>>> made worse by this patch).  I'll file an bug to fix it and hopefully
>>> add detection for this.  I think we don't need to take CLDG_lock in a
>>> safepoint and should prevent doing so, but things like this are more
>>> reliable to do with computers than visual inspection. Can you add your
>>> suggestions to my RFE? https://bugs.openjdk.java.net/browse/JDK-8213150
>>>
>>> The CLDG lock can be shared by non-java threads and java threads,
>>> which is the point.  There may be other locks though.
>>>>> The locks added in this patch set though are for the NonJavaThreads,
>>>>> who do not do safepoint checks.  The NonJavaThreads that acquire
>>>>> these locks use the STS joiner mechanism which disallows safepoints
>>>>> while being held (and since they are non Java threads, do not check
>>>>> for safepoints themselves).
>>>>>
>>>>> This is how it's going to look for the ZGC caller:
>>>>>
>>>>> {
>>>>> SuspendibleThreadSetJoiner sts_joiner;
>>>>> // Unlink the classes.
>>>>> MutexLockerEx ml(ClassLoaderDataGraph_lock);
>>>>> unloading_occurred =
>>>>> SystemDictionary::do_unloading(ZStatPhase::timer(),
>>>>> true /* do_cleaning */););
>>>>> }
>>>> Somehow I missed the creation/invention of the STS joiner mechanism.
>>>>
>>> Me too!   It's in gc/shared but it's really runtime code, except
>>> people in runtime didn't know about it because it's used by GC threads.
>>>
>>> Let me know if you have more questions and can review this code.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>> David
>>>>
>>>>>> Further, are these locks acquired by non-JavaThreads such that the
>>>>>> VMThread may be delayed whilst a safepoint is active?
>>>>> Yes, theoretically they could delay the VMThread from getting to a
>>>>> safepoint or doing its work while in a safepoint but the threads
>>>>> that take out these locks only hold them for short durations.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen