On Tue, 23 Feb 2021 23:52:15 GMT, Daniel D. Daugherty <[email protected]>
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
