RFR: 8225702(13): Safepoint counter can't be used for safepoint detection

[David Holmes]

Hi Robbin,

On 14/06/2019 6:26 pm, Robbin Ehn wrote:
> Hi all, please review.
> 
> This bug fix introduced a new useage of the safepoint counter.
> Which does not work with the later pushed faster safepoint code.
> https://bugs.openjdk.java.net/browse/JDK-8215889
> http://hg.openjdk.java.net/jdk/jdk12/rev/4b469f5f4bf2
> 
> In faster safepoint we must publish the new safepoint counter before arming
> and publish the inactive counter after. Reading the safepoint counter 
> without
> having first looked at your local poll can causes faulty assumption about
> safepoint state.

Can you elaborate on exactly how that happens. Naively if you check the 
counter and its changed then you know there has been a change to the 
safepoint state.

> We add a new safepoint id instead which work reliable and we hide the 
> safepoint
> counter getter. (JNI fast get can still use safepoint counter, so it's not
> completely hidden) And provide a simple class to track safepoint state.

Does JNI need to use the counter rather than the new id? We seem to have 
some redundancy/overlap between the counter and the id - so do we really 
need both? Throw in the actual _safepoint_state and we have three bits 
of state information ...

> Also it makes no sense for a normal user that JFR/JMC safepoint id are 
> only odd,
> we instead use this new id which is just incremented for each safepoint.
> 
> Code:
> http://cr.openjdk.java.net/~rehn/8225702/v1/webrev/index.html

src/hotspot/share/runtime/safepoint.hpp

Why did you make thread_not_running an instance method?

src/hotspot/share/runtime/safepoint.cpp

  418   // Record state
  419   _state = _synchronized;
  420
  421   OrderAccess::fence();
  422
  423   // Set the new id
  424   ++_safepoint_id;

Under the assumption that the threads/code that uses the new 
SafepointStateTracker do not participate in the safepoint protocol, is 
there a risk of seeing a stale _safepoint_id, or racing with the 
increment? It's not clear to me under what conditions the 
SafepointStateTracker::safepoint_state_changed() calls will be made.

Thanks,
David
-----

> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8225702
> 
> Passes JFR test and t1-3.
> 
> Thanks, Robbin