[jdk11u-dev] RFR: 8214097: Rework thread initialization and teardown logic

[SUN Guoyun]

On Tue, 19 Dec 2023 10:56:32 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

> @sunny868 Why are you backporting this patch to OpenJDK 11u? It's an intrusive patch that changes thread initialization which isn't suitable for backport at this stage in the lifecycle of JDK 11. There must be a **very** good reason to even consider it.

runtime/InternalApi/ThreadCpuTimesDeadlock.java can trigger exception occasionally. I traced the cause:

<pre>
// src/hotspot/os/linux/os_linux.cpp
// be called by com.sun.jmx.mbeanserver.JmxMBeanServer

6171 static jlong fast_cpu_time(Thread *thread) {                                    
6172     clockid_t clockid;                                                          
6173     int rc = os::Linux::pthread_getcpuclockid(thread->osthread()->pthread_id(), 
6174                                               &clockid);        //  thread->osthread()  maybe NULL or pthread_id() is 0.
6175     if (rc == 0) {                                                              
6176       return os::Linux::fast_thread_cpu_time(clockid);                          
6177     } else {                                                                    
6178       // It's possible to encounter a terminated native thread that failed      
6179       // to detach itself from the VM - which should result in ESRCH.           
6180       assert_status(rc == ESRCH, rc, "pthread_getcpuclockid failed");           
6181       return -1;                                                                
6182     }                                                                           
6183 } 
</pre>

Here.  Line 6173 `thread->osthread()` is a possibility of being nullptr because the non-java-thread was placed on the `_the_list` too early.

<pre>
// share/runtime/thread.cpp
1309 NonJavaThread::NonJavaThread() : Thread(), _next(NULL) {                        
1310   // Add this thread to _the_list.                                              
1311   MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); 
1312   _next = _the_list._head;                                                      
1313   OrderAccess::release_store(&_the_list._head, this);          // Do early. It should be done after thread state is RUNNED
1314 } 
</pre>

I see this patch do `add thread to _the_list` in `NonJavaThread::pre_run()`, which can make sure the non-java-thread has finished `set_osthread` and `osthread->set_pthread_id` on `os::create_thread`. The correct order is

<pre>
os::create_thread(Thread* thread ...) {
      thread->set_osthread(osthread);
      pthread_create(&tid, &attr, (void* (*)(void*)) thread_native_entry, thread);
      osthread->set_pthread_id(tid);
      wait until osthread state equal INITIALIZED.
}

thread_native_entry(Thread *thread) {
  osthread->set_state(INITIALIZED);
  wait until os::start_thread() // set thread state RUNNED
  thread->call_run();  // call NonJavaThread::pre_run(), add thread to _the_list
}
</pre>

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

PR Comment: https://git.openjdk.org/jdk11u-dev/pull/2411#issuecomment-1862634670