RFR: 8215491: ICStubInterface::finalize finds zombie nmethod with ZGC concurrent class unloading

[coleen.phillimore at oracle.com]

On 12/17/18 2:57 PM, Erik Österlund wrote:
> Hi Dean,
>
> On 2018-12-17 20:27, dean.long at oracle.com wrote:
>> On 12/17/18 8:56 AM, Erik Österlund wrote:
>>> Hi,
>>>
>>> Today, when converting a not_entrant nmethod to zombie, the sweeper 
>>> first clears the IC stubs of that nmethod, and then make_zombie(). 
>>> With concurrent class unloading, this ordering is a bit unfortunate. 
>>> Between clearing the IC stubs and making it zombie, the concurrent 
>>> GC threads may hit a timing window where they will create IC stubs 
>>> for concurrently cleaning the ICs of this still is_alive() nmethod. 
>>
>> Do we normally have to creates stubs to clean an IC?
>
> Yes if you clean an IC in an nmethod that is_alive(), we currently 
> have to do that using IC stubs. I have some ideas going forward for 
> how we can stop doing that, but that is outside of scope for now.
>
>>
>>> The result is that during safepoint cleanup, when we finalize the IC 
>>> stubs, we find one associated with a zombie.
>>>
>>> The unregistering of the nmethod from the GC will block during 
>>> concurrent nmethod unloading, so clearing the IC stubs after the 
>>> nmethod has become zombie is a lot more sane as there can not be any 
>>> such races then.
>>>
>>
>> Where is the unregistering happening?
>
> Inside of make_zombie() we unregister_nmethod() on CollectedHeap. In 
> there, ZGC makes sure we wait until concurrent nmethod unloading is 
> over before continuing.

Can you move clean_ic_stubs() inside of make_not_entrant_or_zombie() to 
maintain the order dependence?
thanks,
Coleen
>
>>
>> Doing the clearing after it's a zombie does sound safer.
>
> Agreed!
>
> Thanks for reviewing.
>
> /Erik
>
>> dl
>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8215491
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8215491/webrev.00/index.html
>>>
>>> Thanks,
>>> /Erik
>>