RFR: 8234426: Sweeper should not CompiledIC::set_to_clean with ICStubs for is_unloading() nmethods

[Stefan Karlsson]

Hi Erik,

After discussing this with Erik, I think this looks good. Just to 
recapture some of my questions and thoughts:

The nmethod can be in three interesting states when the sweeper executes 
this code:
1) is_alive && !is_unloading
2) is_alive && is_unloading
3) !is_alive && is_unloading

The transition from (2) to (3) happens when the GC is unloading the 
nmethod concurrently while the sweeper is processing the nmethod. It's 
only interesting to bring up state (3), because this is the reason why 
we can't assert that the nmethod is_alive.

The interesting transition happens between (1) and (2), and this 
transition is well-synchronized between the GC and sweeper by means of 
the synchronization protocol in nmethod::is_unloading.

In (1) all oops are good and the nmethod is guaranteed to not be unloaded.
In (2) at least one of the oops are bad and the nmethod is on its way to 
become unloaded

The sweeper communicates that it's processing an nmethod by exposing it 
to the root set of the threads. We are OK with exposing (1) nmethods, 
but we must make sure we never expose (2) nmethods.

The only time the sweeper-processed nmethod is exposed to the root set 
of the threads is when we safepoint. The only path that can safepoint in 
set_to_clean(bool in_use) is when we're using ICStubs. However, we don't 
need to use ICStubs when an nmethod is not "in use". is_unloading 
nmethods (2) are not "in use", otherwise they would have been found by 
root scanning and/or entry barriers and would have stayed !is_unloading. 
Therefore the code was changed to set_to_clean(!is_unloading()).

Thanks,
StefanK

On 2019-11-22 11:49, erik.osterlund at oracle.com wrote:
> Hi,
>
> When the sweeper processes an nmethod, it will clean inline caches if 
> it is_alive().
> Today, the cleaning will utilize transitional states (using ICStubs) 
> if the nmethod is_alive(),
> which is always true for the sweeper. If it runs out of ICStubs, it 
> might have to safepoint
> to refill them. When it does, the currently processed nmethod might be 
> is_unloading().
> That is not a problem for the GC per se (safepoint operation fusing 
> with mark end), but it
> is a problem for heap walkers that get confused that an nmethod 
> reachable from a thread is unloading
> and hence has dead oops in it. This sweeper nmethod is the *only* 
> nmethod that violates an
> invariant that nmethods reachable from threads (Thread::nmethods_do) 
> are not unloading.
>
> By simply changing the condition to not use ICStubs when the nmethod 
> is_unloading(), we
> get this neat invariant, and code gets less confused about this.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8234426
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8234426/webrev.00/
>
> Thanks,
> /Erik