On Fri, 17 May 2024 19:52:00 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> After calling getLocks(), there may still be attempts to enter locks. The >> locks should already be held. I filed >> [JDK-8330193](https://bugs.openjdk.org/browse/JDK-8330193) to assert that. >> However, even when entering an already entered lock, we still need to grab >> dbgRawMonitor. I found that out the hard way when there were deadlocks on >> dbgRawMonitor because it was not entered it here. > > I think expose dbgRawMonitor outside of util.c is wrong (and use > gdata->rankedMonitors outside of utils.c too). > If it's really required, it would be better to add functions to > disable/enable monitor locks. > But I still don't understand why we need this (and what is JDK-8330193 about) > Both JVMTI raw monitors and DebugRawMonitor support re-entrance, so this > thread may enter the locks again (and other threads will wait for the locks > if they try to enter them). > And I don't see how we can get deadlocks on dbgRawMonitor, could you please > elaborate on that. The comment above getLocks() pretty much explains it. Before doing any thread suspension we can't allow any other thread to hold a lock that might be needed during suspension. This is why getLocks() is used to grab all these locks in advanced. If they were not grabbed in advanced and one of the locks was held by another thread that got suspended, then eventually the current thread (the one that initiated the thread suspension) would block on the suspended thread, which means it would never get resumed, so we have a deadlock. If we don't include dbgRawMonitor in this set of locks and we suspend a thread while it holds it, this prevents the current thread from doing a debugMonitorEnter on any lock, even ones it already holds. Note we can't check if the current thread owns a lock without grabbing dbgRawMonitor. In fact the main purpose of dbgRawMonitor is to allow the current thread to check if it owns the monitor. I don't disagree that it's a bit of a wart that dbgRawMonitor is exposed in this manner. I just don't see a way around it. I can hide gdata->rankedMonitors in dbgRawMonitor_lock/unlock() if you want, but I can't see of way to not export dbgRawMonitor_lock/unlock() in some fashion, possibly with a different name. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605602137
