On Wed, 17 Feb 2021 03:47:04 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 three additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8241403
> - Address coleenp CR0 comments.
> - 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware
Hi Dan,
Sorry but I have a lot of issues with this. After thinking about it a lot I
don't think the current approach is what is needed. To repeat what I wrote in
one of the comments I think the simple fix here is to replace the use of
Threads_lock in the caller with suitable use of a TLH, and then replace the
assert_locked_or_safepoint(Threads_lock) with
assert(is_JavaThread_protected(t)) where is_JavaThread_protected checks the
target for either being the current thread or included in a TLH of the current
thread. I don't think get_thread_name() should try to protect the target as
that is the responsibility of the caller.
Thanks,
David
src/hotspot/share/runtime/thread.cpp line 486:
> 484:
> 485: // Is the target JavaThread protected by this Thread:
> 486: bool Thread::is_JavaThread_protected(const JavaThread* p) {
Why is this a function on Thread instead of JavaThread??
src/hotspot/share/prims/jvmtiTrace.cpp line 283:
> 281: // If the target JavaThread is not protected, then we return the
> 282: // specified non-NULL string:
> 283: return thread->as_Java_thread()->get_thread_name("<NOT FILLED IN>");
That's not what the original logic is doing. We only return "NOT FILLED IN" if
we find that the java.lang.Thread oop does not have a name set. That's not
normally possible but this code is intended to be 100% safe, hence the check.
(And it may be possible to see null during the attach process ...)
I would expect the target thread to already be protected when this method is
called in any case. So while this code partially duplicates
JavaThread::get_thread_name() I'm not convinced we need to change this code to
use JavaThread::get_thread_name() instead.
src/hotspot/share/runtime/thread.cpp line 490:
> 488: // Current thread is always protected:
> 489: return true;
> 490: }
This is true but it is awkward that we are going to manifest Thread::current
multiple times when using this - see below.
src/hotspot/share/runtime/thread.cpp line 2547:
> 2545: // for such that this method never returns NULL.
> 2546: const char* JavaThread::get_thread_name(const char* default_name) const
> {
> 2547: Thread* thread = Thread::current();
So you manifest Thread::current() and then is_JavaThread_protected will
manifest it again.
src/hotspot/share/runtime/thread.cpp line 2548:
> 2546: const char* JavaThread::get_thread_name(const char* default_name) const
> {
> 2547: Thread* thread = Thread::current();
> 2548: ThreadsListHandle tlh(thread);
I would only expect this code to create a TLH if the target is not protected,
otherwise why do we need to search through the existing thread-lists as it
would suffice to see if the target is within the newly created TLH ??
I expected the basic logic here to be:
`if (thread is protected) return name;
else { protect the thread; return name; }`
though if it is cheaper to create a TLH than to search the existing ones, then
we may as well just create the TLH, check that the target has been captured and
then return its name.
That said we need to carefully examine where the taking of the Threads_lock
would occur in the thread termination sequence, to understand what observable
states are possible with respect to the target having a threadObj. (Though this
may have already changed with the introduction of Thread-SMR.)
That all said I'm not so sure the only thing we actually need to do for this
issue is to replace the use of Threads_lock, in the caller, with suitable TLH
usage, and then in get_thread_name() assert that the target is in fact
protected.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2535
