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

[David Holmes]

On Wed, 17 Feb 2021 03:47:04 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 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