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

[Erik Österlund]

Hi Coleen,

On 2018-12-17 22:31, coleen.phillimore at oracle.com wrote:
> 
> 
> 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?

Sure. The implication of that is that the sweeper will redundantly check 
for IC stubs to clear on unloaded nmethods that can't have any. But I 
agree that the model is more clear if it is inside of 
make_not_entrant_or_zombie(), and think that is worth the cost.

New webrev:
http://cr.openjdk.java.net/~eosterlund/8215491/webrev.01/

Thanks for reviewing.

/Erik

> 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
>>>
>