RFR: 8343741: SA jstack --mixed should print information about VM locks [v7]

Wed Nov 20 05:50:18 UTC 2024

On Tue, 19 Nov 2024 18:30:37 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> Hi
>> Could you please review the the fix that add locks information into SA jhsdb stack --mixed output.
>> 
>> Here is the motivation for this rfe and explanation why I add it into SA now.
>> 
>> The information about current owners of Hotspot Mutex is often very useful for dealock investigations.
>> 
>> The jcmd usually doesn't work because VM can't reach or exit safepoints. So it doesn't respond to jcmd.
>> 
>> The SA 'jstack --mixed' provides information about stacktraces on Java and non-Java Threads. So having information about locks along with stack traces might significantly help to identify issues.
>> 
>> The gdb allows to provide stacktraces, but the debug symbols are required to get info about locks. These symbols are often absent during execution.
>> Also the debugger solution is OS specifc.
>> 
>> The significant part of fix is refacotorrng of mutex_array to be vmStructs compatible.
>> 
>> The adding support of non-JavaThreads into SA might be implemented later to obtain more info about their names.
>> The example of output:
>> 
>> [2024-11-06T21:32:48.897414435Z] Gathering output for process 1620563
>> Attaching to process ID 1620533, please wait...
>> Debugger attached successfully.
>> Server compiler detected.
>> JVM version is 24-internal-adhoc.lmesnik.open
>> Deadlock Detection:
>> 
>> No deadlocks found.
>> 
>> Internal VM Mutex Threads_lock is owned by Unknnown thread (Might be non-Java Thread) with address: 0x00007f8cf825b190
>> Internal VM Mutex Compile_lock is owned by LockerThread with address: 0x00007f8cf8309a00
>> ----------------- 1620559 -----------------
>> "C1 CompilerThread4" https://github.com/openjdk/jdk/pull/28 daemon prio=9 tid=0x00007f8c300566a0 nid=1620559 runnable [0x0000000000000000]
>> java.lang.Thread.State: RUNNABLE
>> JavaThread state: _thread_blocked
>> 0x00007f8cff11e88d syscall + 0x1d
>> 0x00007f8cfe6c99de LinuxWaitBarrier::wait(int) + 0x8e
>> 0x00007f8cfe2be409 SafepointMechanism::process(JavaThread*, bool, bool) + 0x79
>> 0x00007f8cfd53ea91 ThreadInVMfromNative::ThreadInVMfromNative(JavaThread*) + 0xc1
>> 0x00007f8cfd534a00 ciEnv::cache_jvmti_state() + 0x30
>> 0x00007f8cfd679614 CompileBroker::invoke_compiler_on_method(CompileTask*) + 0x204
>> 0x00007f8cfd67adc8 CompileBroker::compiler_thread_loop() + 0x5c8
>> 0x00007f8cfdb4426c JavaThread::thread_main_inner() + 0xcc
>> 0x00007f8cfe5a0bbe Thread::call_run() + 0xbe
>> 0x00007f8cfe16813b thread_native_entry(Thread*) + 0x12b
>> .....
>> ----------------- 1620554 -----------------
>> "LockerThread" https://...
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed spaces

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 58:

> 56: 
> 57:     f = type.getField("_owner");
> 58:     ownerFieldOffset = f.getOffset();

Rather than repurpose `f` I could suggest declaring nameField and ownerField. Makes the code a bit easier to read.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 61:

> 59:     mutex_array = type.getAddressField("_mutex_array");
> 60:     maxNum = type.getCIntegerField("_num_mutex").getJInt();
> 61: 

empty line not needed.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 76:

> 74:   public static int maxNum() { return maxNum; }
> 75: 
> 76: 

empty lines not needed

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMLocksPrinter.java line 54:

> 52:             if (thread.getAddress().equals(addr)) {
> 53:                 return thread.getThreadName();
> 54:             }

Instead of searching for the thread address in the list of threads, can't you just create a JavaThread using Threads.createJavaThreadWrapper(addr) and directly fetch the thread name from it?

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMLocksPrinter.java line 58:

> 56:         return "Unknown thread";
> 57:     }
> 58:     public void printVMLocks() {

Add newline before this line.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/PStack.java line 85:

> 83: 
> 84:          VMLocksPrinter vmLocksPrinter = new VMLocksPrinter(out);
> 85:          vmLocksPrinter.printVMLocks();

It would be best to catch all exceptions here and continue on after a warning message. We do the same in the loop below as we iterate over each thread. This is a safety net in case some structures are currently in an inconsistent state.

test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackPrintVMLocks.java line 32:

> 30: /**
> 31:  * @test
> 32:  * @summary Test verifies that jstack --mixed print information about VM locks

Suggestion:

 * @summary Test verifies that jstack --mixed prints information about VM locks

test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackPrintVMLocks.java line 50:

> 48:         try {
> 49:             theApp = new LingeredAppWithLockInVM();
> 50:             String classPath = System.getProperty("test.class.path");

Unused

test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackPrintVMLocks.java line 51:

> 49:             theApp = new LingeredAppWithLockInVM();
> 50:             String classPath = System.getProperty("test.class.path");
> 51:             LingeredApp.startAppExactJvmOpts(theApp,

Why use startAppExactJvmOpts and limit testing to just this one config (with vm.flagless)? Can't these be added arguments?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849552939
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849553124
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849553513
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849559366
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849559568
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849549464
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849565537
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849571429
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1849570238