On Tue, 19 Nov 2024 18:30:37 GMT, Leonid Mesnik <lmes...@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
