RFR: 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware [v5]

[David Holmes]

On Tue, 23 Feb 2021 23:52:15 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> A minor fix to add a new function:
>> 
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>> 
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>> 
>> This fix is tested via a Mach5 Tier[1-8] run.
>
> Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8241403
>  - Address dholmes-ora CR2 comments.
>  - Address dholmes-ora CR1 comments.
>  - Merge branch 'master' into JDK-8241403
>  - Address coleenp CR0 comments.
>  - 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware

Hi Dan,

Sorry still some issue here.

Okay let me back up again. The old code for get_thread_name() thinks it is only safe to access the name of another thread if we hold the Threads_lock or we are at a safepoint. That isn't true as I've pointed out before - there are other ways for the access to be safe. And as a result of this, code calling get_thread_name() would acquire the Threads_lock just to get past the assertion e.g the CompilerBroker code. I've now ascertained that the CompilerBroker calls to get_thread_name() are perfectly safe by construction due to holding the CompilerThread_lock and/or dealing with an immortal thread.

The intent of filing this issue was for get_thread_name() to also (instead?) recognise that it is safe if the target is protected by a TLH - thus relaxing the original constraint by adding another checkable condition - and to recognise that a handshake also makes things safe. I didn't recognise at the time that these are still overly strong requirements.

So the change you are proposing is to replace the "needs the Threads_lock" condition with "must have an active TLH" condition when calling get_thread_name(). So now the unnecessary use of the Threads_lock in the CompileBroker is replaced by unnecessary use of the TLH - which is unfortunate IMO but a fix for that is out of scope for this change.

Also the management code changes now require use of a TLH  - but there is a bug in replacing the Threads_lock usage: see below!

As a Threads_lock->TLH conversion for users of get_thread_name() I can accept the change in its current form. But it needs to restore the asserts in the CompileBroker code as commented below and fixing the Threads_lock bug in the management code.

Thanks,
David

src/hotspot/share/compiler/compileBroker.cpp line 1115:

> 1113:       if (ct == NULL) break;
> 1114:       ThreadsListHandle tlh;
> 1115:       if (!tlh.includes(ct)) break;

This change raised a red flag because if the new thread already terminated it should decrement the number of compiler threads on its way out, but we are skipping the increment by breaking here. So that prompted me to look closer at the bigger picture for these compiler threads and their lifecycle. The current code is called whilst holding the CompilerThread_lock, which means that the new thread can't terminate because it has to also acquire the CompilerThread_lock to decide whether it needs to terminate. So access to ct is guaranteed to be safe and no TLH is needed at all - other than because get_thread_name() requires it. So this should be a simple assertion afterall.

src/hotspot/share/compiler/compileBroker.cpp line 1136:

> 1134:       if (ct == NULL) break;
> 1135:       ThreadsListHandle tlh;
> 1136:       if (!tlh.includes(ct)) break;

Ditto.

src/hotspot/share/runtime/thread.cpp line 2955:

> 2953: void Threads::threads_do(JavaThreadIteratorWithHandle* jtiwh_p, ThreadClosure* tc) {
> 2954:   java_threads_do(jtiwh_p, tc);
> 2955:   non_java_threads_do(tc);

You should still have

(Threads_lock);```

for non-JavaThread access.

src/hotspot/share/services/management.cpp line 1704:

> 1702:   {
> 1703:     JavaThreadIteratorWithHandle jtiwh;
> 1704:     Threads::threads_do(&jtiwh, &ttc);

Same comments as above re need for Threads_lock.

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

Changes requested by dholmes (Reviewer).

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