On Thu, 21 Nov 2024 05:08:06 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: > > SA test doesn't work with ZGC Instead of putting this inside of Mutex, could you instead create a class that is the container of this array? I think that would make a cleaner split between the code that operates on a specific Mutex vs code that is dealing with the set of registered Mutexes. Maybe name it MutexSet, MutexList, MutexArray, RegisteredMutexes, ...? If you don't like that suggestion I'd prefer if you fixed the nits I mention below. src/hotspot/share/runtime/mutex.cpp line 536: > 534: } > 535: #endif // ASSERT > 536: // Print all mutexes/monitors that are currently owned by a thread; > called Please, leave a blank line between the pre-existing functions and the ones you are moving. Suggestion: // Print all mutexes/monitors that are currently owned by a thread; called src/hotspot/share/runtime/mutex.hpp line 54: > 52: class Mutex : public CHeapObj<mtSynchronizer> { > 53: > 54: friend class VMStructs; Suggestion: friend class VMStructs; src/hotspot/share/runtime/mutex.hpp line 138: > 136: > 137: public: > 138: static void add_mutex(Mutex* var); This adds a function to a part of the class that previously only held variables. Could this move to a block where we have functions instead? src/hotspot/share/runtime/mutex.hpp line 207: > 205: // by fatal error handler. > 206: static void print_owned_locks_on_error(outputStream* st); > 207: static void print_lock_ranks(outputStream* st); This splits print_on_error from print_on. Could you move this before print_on_err and make sure that there's a blank line before print_on_error. ------------- Changes requested by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22171#pullrequestreview-2450579123 PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1851562349 PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1851564286 PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1851567414 PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1851568394
