RFR(S): 8200374: Add ThreadsSMRSupport::verify_hazard_pointer_scanned() to verify threads_do()

[Daniel D. Daugherty]

Greetings,

During my testing where I restored the bug in 
JavaThread::verify_not_published()
to verify that the new function works, I made a few minor improvements:

$ hg diff
diff -r df946ef9863e src/hotspot/share/runtime/thread.cpp
--- a/src/hotspot/share/runtime/thread.cpp    Wed Mar 28 20:16:43 2018 -0400
+++ b/src/hotspot/share/runtime/thread.cpp    Thu Mar 29 12:18:46 2018 -0400
@@ -3438,7 +3438,9 @@
    // Someday we could have a table or list of all non-JavaThreads.
    // For now, just manually iterate through them.
    tc->do_thread(VMThread::vm_thread());
-  Universe::heap()->gc_threads_do(tc);
+  if (Universe::heap() != NULL) {
+    Universe::heap()->gc_threads_do(tc);
+  }
    WatcherThread *wt = WatcherThread::watcher_thread();
    // Strictly speaking, the following NULL check isn't sufficient to 
make sure
    // the data for WatcherThread is still valid upon being examined. 
However,
diff -r df946ef9863e src/hotspot/share/runtime/threadSMR.cpp
--- a/src/hotspot/share/runtime/threadSMR.cpp    Wed Mar 28 20:16:43 
2018 -0400
+++ b/src/hotspot/share/runtime/threadSMR.cpp    Thu Mar 29 12:18:46 
2018 -0400
@@ -580,6 +580,8 @@
  // the Thread-SMR protocol.
  void ThreadsSMRSupport::verify_hazard_pointer_scanned(Thread *self, 
ThreadsList *threads) {
  #ifdef ASSERT
+  assert(threads != NULL, "threads must not be NULL");
+
    // The closure will attempt to verify that the calling thread can
    // be found by threads_do() on the specified ThreadsList. If it
    // is successful, then the specified ThreadsList was acquired as


The Universe::heap() NULL check in Threads::non_java_threads_do() is
needed because the particular code path to verify_not_published() is
very early in VM bringup... without it we always fail on a NULL ptr
deref (which isn't interesting)...

The new assert() in verify_hazard_pointer_scanned() is just the usual
verify your input parameters...

Dan

On 3/29/18 9:09 AM, Daniel D. Daugherty wrote:
> Erik,
>
> Thanks for the quick review!
>
>
> On 3/29/18 3:00 AM, Erik Österlund wrote:
>> Hi Dan,
>>
>> This looks great.
>
> Good to hear especially since you wrote most of it... :-)
>
>
>> I will sleep much better at night with this 1:1 verification. :)
>
> It does tighten things up nicely!
>
> Dan
>
>
>>
>> Thanks,
>> /Erik
>>
>>
>> On 2018-03-29 03:52, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have a (mostly) small enhancement for Thread-SMR:
>>>
>>> JDK-8200374 Add ThreadsSMRSupport::verify_hazard_pointer_scanned() 
>>> to verify threads_do()
>>> https://bugs.openjdk.java.net/browse/JDK-8200374
>>>
>>> Here's the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8200374-webrev/0_for_jdk_hs_open/
>>>
>>> This is Erik O's improvement on the assertion added by the following
>>> bug fix (with some minor tweaking done by me):
>>>
>>> JDK-8199813 SIGSEGV in ThreadsList::includes()
>>> https://bugs.openjdk.java.net/browse/JDK-8199813
>>>
>>> Summary of the changes:
>>>
>>> - Replace the assertion that I added in JDK-8199813 with a closure
>>>   based function that verifies the threads_do() contract depended
>>>   on by Thread-SMR.
>>> - Add ThreadsSMRSupport::verify_hazard_pointer_scanned() to verify
>>>   that the calling thread's hazard pointer is scanned by threads_do().
>>>   The new function is called from both ThreadsSMRSupport::acquire_*()
>>>   functions.
>>> - Refactor the non-JavaThread part of Threads::threads_do() into
>>>   Threads::non_java_threads_do() so that the non-JavaThread part
>>>   can also be called by other threads_do() functions. Yes, the
>>>   Threads::threads_do() contract is still to scan every thread in
>>>   the system.
>>> - Add hooks for a "tracing sampler thread" to be optionally scanned
>>>   by Threads::non_java_threads_do().
>>>
>>> This fix has gone thru a couple of Mach5 builds-tier1, jdk-tier[1-3],
>>> and hs-tier[1-3] runs. I've also started my usual 24+ hour Thread-SMR
>>> stress testing run on my Solaris-X64 server.
>>>
>>> Just to be extra sure, I backed out the fix from JDK-8199813 to
>>> JavaThread::verify_not_published() (which started this round of
>>> Thread-SMR fixes) and we catch the issue in the new function
>>> ThreadsSMRSupport::verify_hazard_pointer_scanned().
>>>
>>> Thanks, in advance, for any comments, suggestions, or feedback.
>>>
>>> Dan
>>>
>>
>
>