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

[David Holmes]

Hi Dan,

On 18/02/2021 3:36 am, Daniel D.Daugherty wrote:
> On Wed, 17 Feb 2021 07:32:46 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> 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
> 
> @dholmes-ora - I'm really glad that I waited for your review! Thanks for taking the time.

I think there are more issues with the existing code that then impact 
any changes.

The general rule to operate on a target thread is that it must not be 
able to terminate (or more strongly be deallocated) whilst you may 
operate on it, but there are many ways this can be true:

- current thread
- at a safepoint
- in a handshake
- captured in a TLH
- target thread is immortal
- current thread holds a resource that the target needs before it exits

This last category is very general and can't really be captured by an 
assertion. Holding the Threads_lock was one old example (and could be 
asserted), but there are other possibilities including:

- current thread holds the ObjectMonitor of the target's j.l.Thread instance
- current thread is "synchronizing" with the target and the target can't 
proceed until after the current thread releases it

So I think the current assertion in get_thread_name is more restrictive 
than is actually necessary (imagine adding a logging statement in the OM 
code that prints the name of the thread it has selected to unpark - 
perfectly safe but no safepoint active and no Threads_lock held). 
Changing that assertion is also more involved than just checking for an 
active TLH as there are many safety conditions, not all of which can be 
checked explicitly.

I think the CompileBroker code was a case of dealing with immortal 
threads (before UseDynamicNumberOfCompilerThreads was introduced) and 
the acquisition of the Threads_lock was simply done to placate the 
assertion. With dynamic compiler threads it is I guess theoretically 
possible that a newly created thread might terminate again before the 
creating thread reaches the get_thread_name() call - but exceedingly 
unlikely. Not sure how to fix that one without splitting apart the 
thread creation and start processes.

Cheers,
David
-----


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