RFR: 8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading

[Erik Österlund]

On Thu, 22 Oct 2020 09:41:38 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> When class unloading is turned off (-XX:-ClassUnloading) we consider all nmethods in the code cache to be "concurrent roots":
> void ZConcurrentRootsIterator::oops_do(ZRootsIteratorClosure* cl) {
> ...
>   if (!ClassUnloading) {
>     _code_cache.oops_do(cl);
>   }
> }
> This means that they are concurrently fixed up. See ZMarkConcurrentRootsTask and ZMarkConcurrentRootsIteratorClosure. Java threads that want to engage with an nmethod, needs to go through an nmethod entry barrier in case the concurrent roots have not been processed yet. When the ZHeapIterator is used, we rely on the fact that the vm operation the heap iteration is running in, has done a pre-step to fix all thread stacks (including "entering" the relevant nmethods"). See code using:
> 
>   // You may override skip_thread_oop_barriers to return true if the operation
>   // does not access thread-private oops (including frames).
>   virtual bool skip_thread_oop_barriers() const { return false; }
> 
> So, this works when we don't use the entire code cache as root. However, when running with -XX:-ClassUnloading, there's no guarantee that the _code_cache.oops_do(cl) call above has run when the heap iteration is executing. So, the heap iteration code could end up reading oops from an nmethod that has not been entered. Currently, this is benign, since we do apply appropriate load barriers to handle this. However, it would be better if we had a clearer model that only entered/disarmed nmethods are visited.
> 
> The proposed patch extends the previous should_disarm_nmethods() bool, into an enum stating what "enter" operation ZNMethodToOopsDoClosure should perform.
> 
> I think the names of the values quite self-explanatory:
>   PreBarrier,
>   VerifyDisarmed,
>   None
> The tricky one is `Disarm`, which is only used by the marking code. It's used to provide an optimized entry barrier for the marking code. However, since the marking code only provides an OopClosure, and not an NMethodClosuer, we have to put the disarming code here. I have some ideas on how to make all this simpler, but I first want some simplifications that it requires some other patches around our weak root handling to trickle in first.

Looks good in general. Found 2 VerifyDisarmed that should probably be None instead as I don't think they ever touch an nmethod. If you agree, then I am good with the rest.

src/hotspot/share/gc/z/zOopClosures.hpp line 74:

> 72: 
> 73:   virtual ZNMethodEntry nmethod_entry() const override {
> 74:     return ZNMethodEntry::VerifyDisarmed;

I can't see that this will trigger any verification. The ZPhantomKeepAliveOopClosure is used in ZProcessWeakRootsTask, and it does not visit nmethods. Even if it did, it could still not assert that the visited nmethods are not disarmed, because it could have a single oop that is live, despite the nmethod not being entered.

src/hotspot/share/gc/z/zOopClosures.hpp line 84:

> 82: 
> 83:   virtual ZNMethodEntry nmethod_entry() const override {
> 84:     return ZNMethodEntry::VerifyDisarmed;

Same here, except the ZPhantomCleanOopClosure is used from ZProcessConcurrentWeakRootsTask which also does not visit any nmethods. Similar argument that if it did, it still would not make sense to verify that nmethods have been disarmed.

-------------

Changes requested by eosterlund (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/801