RFR: 8288497: add support for JavaThread::is_gc_barrier_detached()

[Daniel D. Daugherty]

On Thu, 16 Jun 2022 07:29:34 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> A trivial fix to add support for JavaThread::is_gc_barrier_detached() which allows
>> us to add checks to detect failures like:
>> 
>>     JDK-8288139 JavaThread touches oop after GC barrier is detached
>>     https://bugs.openjdk.org/browse/JDK-8288139
>> 
>> This fix along with the fix for JDK-8288139 has been tested in Mach5 Tier[1-8].
>> There are no related failures in Mach5 Tier[1-7]; Mach5 Tier8 is still running.
>
> Hi,
> 
> Except for the few lines of code between:
> BarrierSet::barrier_set()->on_thread_detach(p) and ThreadsSMRSupport::remove();
> 
> This new state is identical to the result of "ThreadsSMRSupport::get_java_thread_list()->includes(p)".
> The JavaThread is attached to the barrier set when we add it to the ThreadsList and detached when we remove it.
> 
> If we have a safepoint poll between (done with Threads_lock held):
> BarrierSet::barrier_set()->on_thread_detach(p);
> ->
> p->set_terminated(JavaThread::_thread_terminated);
> 
> We have a bug. This means that this new state is not even possible to observe from another thread (with target safe).
> So for any other thread there is no difference, which means we have new global state just for a few lines of code while we are terminating with Threads_lock held.
> 
> So I don't think this one place needs a new global state.
> 
> Note that for a safe target thread the terminated flag gives the same result as:
> "ThreadsSMRSupport::get_java_thread_list()->includes(p)"
> 
> So jt->is_terminated() can be implemented with "ThreadsSMRSupport::get_java_thread_list()->includes(p)". (again assuming we are not doing some racy, and only asking for threads that are safe/stable)

@robehn's comment from above:

> Except for the few lines of code between:
> BarrierSet::barrier_set()->on_thread_detach(p) and ThreadsSMRSupport::remove();
> 
> This new state is identical to the result of "ThreadsSMRSupport::get_java_thread_list()->includes(p)".

No it's not equivalent. If I replace is_gc_barrier_attached()'s implementation
with your check, then it would NOT catch the code that's in the wrong place
and is being fixed by JDK-8288139. Setting the new _terminated field state
exactly where I do so in this fix is what allowed me to prove that the code
that I moved in JDK-8288139 is in the wrong place.

> This means that this new state is not even possible to observe from another thread (with target safe).

This new state is NOT for another thread to use. This new state is for the calling
thread to use to make sure that it doesn't use oops after the GC barrier is
detached.

> So I don't think this one place needs a new global state.

Not for the long term. @dholmes-ora and I are hoping that the GC folks eventually
add some logic in the GC subsystem that catches this condition. If that is done, then
the this is_gc_barrier_detached() support code can all be removed.

> Note that for a safe target thread the terminated flag gives the same result as:
> "ThreadsSMRSupport::get_java_thread_list()->includes(p)"
> 
> So jt->is_terminated() can be implemented with "ThreadsSMRSupport::get_java_thread_list()->includes(p)".
> (again assuming we are not doing some racy, and only asking for threads that are safe/stable)

I don't think we want to retire the _terminated field in this bug fix. And the
replacement code is much more expensive as the ThreadsList grows due to
the search time.

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

PR: https://git.openjdk.org/jdk19/pull/20