8304824: NMT should not use ThreadCritical #20852
Conversation
|
👋 Welcome back roberttoyonaga! A progress list of the required criteria for merging this PR into |
|
@roberttoyonaga This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 588 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @gerard-ziemski, @jdksjolen, @tstuefe) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@roberttoyonaga To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command. Applicable Labels
|
|
/label serviceability |
|
@roberttoyonaga |
roberttoyonaga
force-pushed
the
JDK-8304824
branch
2 times, most recently
from
7ac88d2 to
a9731e2
Compare
roberttoyonaga
force-pushed
the
JDK-8304824
branch
from
a9731e2 to
97f8296
Compare
|
The GHA |
Webrevs
|
src/hotspot/share/nmt/nmtCommon.cpp
Outdated
| Semaphore NmtGuard::_nmt_semaphore(1); | ||
| intx volatile NmtGuard::_owner((intx) -1); |
There was a problem hiding this comment.
Please add explanatory comments.
src/hotspot/share/nmt/nmtCommon.hpp
Outdated
| #include "runtime/os.hpp" | ||
| #include "runtime/atomic.hpp" | ||
| #include "runtime/javaThread.hpp" |
There was a problem hiding this comment.
Please list in alphabetic order.
src/hotspot/share/nmt/nmtCommon.hpp
Outdated
| NmtGuard() { | ||
| intx const current = os::current_thread_id(); | ||
| intx const owner = Atomic::load(&_owner); | ||
| assert(current != owner, "Lock is not reentrant"); |
There was a problem hiding this comment.
ThreadCritical is reentrant though. We would need to do a lot of testing and/or code analysis to ensure we don't have the possibility of needing a reentrant lock with NMT. The most likely problems would be if we triggered a crash whilst in a locked section of code.
There was a problem hiding this comment.
We have a healthy amount of tests already. We need a wide use of assert_locked and need to take a close look at error reporting. Locking, in NMT, is rather simple.
Error handling: My pragmatic "good enough for this rare scenario" proposal would be:
At the entry of error reporting, if the NMT mutex is locked by the crashing thread, just unlock it manually and try to continue. The chance is high that this will actually work well enough to survive the few mallocs we may do during error reporting.
Reasoning: We use NMT mutex as we did ThreadCritical: rather widely scoped. The time windows where things are out of sync and reentrant use of the same functionality can bite us are small. Moreover, NMT's two subsystems - malloc tracker and mmap tracker - are mostly independent from each other, and even leaving the mmap tracker in an inconsistent state will not prevent us from doing malloc during error reporting. We don't use mmap during error reporting, just malloc. Finally, moreover, NMT malloc code is comparatively simple and stable; mmap code is more complex, and we plan to rework parts of it in the near future.
TL;DR I think just manually releasing the lock in error reporting has a high chance of succeeding. Especially if we disable the NMT report in the hs-err file I am not even sure this is necessary, though. We have safeguards against error reporting locking up (step timeouts and secondary crash guards).
There was a problem hiding this comment.
Thinking through this more, we can probably shorten my advice to this:
When entering error handling, if NMT lock is held by crashing thread and NMT level is detail, switch back to NMT level summary.
That's it.
Reasoning: we only do mallocs inside hs-err generation. For malloc, we only lock inside NMT if the NMT level is detail. And this should never change: malloc is too hot to justify a lock in summary mode.
The only point where possible crashes or deadlocks can occur in error reporting is when doing the NMT report itself - but chances for this to happen are remote, and we have secondary crash handling and error step timeouts to deal with this if it happens.
There was a problem hiding this comment.
Ok I've added some logic in VMError::report to switch to summary mode (I think this is the right place). However, if we need to do error reporting while the NmtGuard is already held, then I think we might still have reentrancy even in summary mode. After the latest commit that replaces MemoryFileTracker's locker with NmtGuard, MemBaseline::baseline_summary() now acquires NmtGuard too.
Because of this I've also made NmtGuard reentrant. Maybe handling reentrancy would be beneficial in future/unforeseen situations as well. But if this is unnecessary, I don't mind removing it.
src/hotspot/share/nmt/nmtCommon.hpp
Outdated
| intx const current = os::current_thread_id(); | ||
| intx const owner = Atomic::load(&_owner); | ||
| assert(current == owner, "NMT lock should be acquired in this section."); |
There was a problem hiding this comment.
Please put the entire body in ifdef DEBUG so we don't execute anything in release builds.
Also the assert should report the values of owner and current when they are not as expected.
|
I plan to look at this start of next week. |
|
Thank you Robert. I think that the |
+1 |
|
@roberttoyonaga Why don't we use a normal hotspot mutex here? |
src/hotspot/share/nmt/nmtCommon.hpp
Outdated
| static void assert_locked(){ | ||
| intx const current = os::current_thread_id(); | ||
| intx const owner = Atomic::load(&_owner); | ||
| assert(current == owner, "NMT lock should be acquired in this section."); |
There was a problem hiding this comment.
How about we try to re-use assert_locked() in constructor and destructor, something like:
NmtGuard() {
assert(!assert_locked(), "Lock is not reentrant");
_nmt_semaphore.wait();
Atomic::store(&_owner, current);
}
~NmtGuard() {
assert(assert_locked(), "NMT lock should be acquired in this section.");
Atomic::store(&_owner, (intx) -1);
_nmt_semaphore.signal();
}
static bool assert_locked(){
intx const current = os::current_thread_id();
intx const owner = Atomic::load(&_owner);
return (current == owner);
}
There was a problem hiding this comment.
I think it's a good idea to use it in the destructor. But based on the discussion above, it seems like we shouldn't assert the lock can't be reentered.
…er. make reentrant.
Hi @tstuefe. I tried using a regular mutex along with
|
Hi @jdksjolen. No problem! Ok I've replaced it. |
Hi @roberttoyonaga, sorry for the sluggish responses, I am swamped atm. Thanks a lot for your work here. I dislike the self-written lock because of duplication and because Mutex gives us infrastructure benefits we don't have here. E.g. locking monitoring, deadlock detection, and things like You run into this problem at the initialization phase. I expected that, and here is where things gets interesting - here and in deadlock detection. I see two better solutions:
So, you could use Mutex and MutexLocker but short-circuit locking if the mutex is still zero. This should work and should be lock/unlock balanced too. I believe MutexLocker already handles this case under the hood. You can call it with a nullptr mutex, and it will not lock. So you don't even have to do anything. Just don't assert.
I prefer (1), since then, locking would work as it does in all other places in hotspot. |
|
Hi @tstuefe,
Ok I've switched to using a regular hotspot
|
|
After switching to a Hotspot Mutex, it looks like the |
What code exactly locks tty_lock? |
This is weird. We print the error in this case via print_error_for_unit_test(), which takes care only to use raw fprintf(stderr) for this very reason... therefore I am interested in understanding the lock semantics, to know if deadlock detection found a real problem or if this is just a weird error setup. |
|
Thank you everyone for the review! |
|
@roberttoyonaga |
|
/sponsor |
|
Going to push as commit 0abfa3b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@tstuefe @roberttoyonaga Pushed as commit 0abfa3b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
We saw some test failures in non-generational ZGC due to this change. That GC is removed now, so there's no worry there. I did however look at all of our remaining usages of I am not that familiar with this code. Is leaving these as they are intentional, or have we introduced a potential bug? Thank you. |
|
Hi @jdksjolen, I originally replaced those instances, but then put them back in because I saw that they are related to chunks allocation/deallocation code where it seemed I'm not certain, but looking at it again, it seems that the |
|
Also, those uses of What do you think @tstuefe? |
At least for prune, that's needed for the chunk pool itself as it would otherwise be accessed concurrently.
OK, I understand. That seems reasonable. |
|
We should get rid of this awful value correcting for arenas. I get headaches trying to think about this every time. To remind everyone including me, some of the stuff is explained here: complex . About locking around os::free - not sure if we can do this. Are all paths inside os::free lock free? It does call NMT. I think all cases inside NMT that touches are lock free, but am not sure. Have to think about this a biot. |
Ah okay should be fine. Both mallocs and frees should be lock free in NMT. In summary mode we just modify counters. In detail mode, we add to the malloc site table, but that one is lock free as well. |
|
Ok so there shouldn't be reentrancy if we use the
So that means we'd need to have both |
Please split this code in two parts. First part removes retired chunks from the arena chunk list and adds them to a new temporary list. Second part goes through the temp list and deletes the chunks. First part under ThreadCritical, second part under |
|
I had to analyze this again, to understand why we need this locking, since my mind slips. I updated my findings in https://bugs.openjdk.org/browse/JDK-8325890 . Please see details there. But I don't think the current locking makes much sense, tbh (even before this patch). Or I don't get it. We have three counters in play here: Upon arena growth, we incremement all four. In two steps - first (A) and (B) under os::malloc, then (C) in the arena code. Upon arena death, we may delete the chunk, in which case we adjust all three counters as well. But since we don't want (B) to include arena sizes, we have several places where we lazily adjust (B) and (A) from the sum of all arena counters (see JBS issue). These adjustments must be synchronized with arena allocations. But we don't do this. The lock does not include (C). For that, the lock would have to be at a different place, inside arena code. We only lock around (A) and (B). I am not sure what the point of this whole thing is. I am also not convinced that it always works correctly. This whole "updating counters lazily" business makes the code brittle. Also, the current locking does not guarantee correctness anyway. There are several places in the coding where we calculate e.g. the sum of all mallocs, but malloc counters are modified (rightly so) out of lock protection. To make matters even more complex, we have not two locks here but three:
Not sure what the point of the latter even is. It certainly does not prevent us from getting reports while underlying data structures are being changed, unless I am overlooking something. It would be very nice if someone other than me could analyze this. |
If this is so brittle and complex, then I wonder what you even get out of us doing the |
@jdksjolen good question. I am not sure who introduced that, may have been Afshin as a fix for some NMT test. |
|
Hi @tstuefe,
Yes, updating all three counters (A/B/C) is not atomic when growing an arena or when destroying an arena. I think the I think this is also confusing because its not consistent whether we count arena sizes as part of malloc. In the NMT report total, arena is counted toward malloc (so we have to subtract mtChunk). But in the reported individual categories, arena is disjunct from malloc. The solution you mentioned in https://bugs.openjdk.org/browse/JDK-8325890 seems like it would solve this consistency problem while also avoiding the double counting problem with mtChunk. I guess the main problem with this is that the change will break things if people are relying on the reported arena values to not overlap with malloc. |
|
The easiest action to take would be to remove the The next best solution would be to make arena size a subset of malloc size, and make mtChunk only responsible for free chunks not yet associated with other NMT categories. Then there would be no need for special adjustments when reporting, and thus no need for the special The third and highest effort solution would be to remove the chunk pooling altogether as suggested in https://bugs.openjdk.org/browse/JDK-8325890. This shares the benefits of solution 2, but also requires performance testing and has consequences that extend beyond NMT.
Yes, still none of the three above solutions solve this. Even with solution 2 or 3 it would be possible to update the arena sizes for each category concurrently with reporting (unless we introduce new locking). |
| @@ -28,6 +28,7 @@ | |||
| #include "memory/allocation.hpp" | |||
| #include "runtime/flags/flagSetting.hpp" | |||
| #include "runtime/mutex.hpp" | |||
| #include "runtime/thread.hpp" | |||
There was a problem hiding this comment.
This include is not needed because there are no uses that require the definition of Thread.
After pull, I am getting circularity errors from runtime/thread.hpp -> jfr/support/jfrThreadLocal.hpp (which now needs runtime/mutexLocker.hpp (-> runtime/thread.hpp)
The include of "runtime/thread.hpp" can instead be placed in "nmt/nmtCommon.hpp" which has a use in the NmtVirtualMemoryLocker constructor.
There was a problem hiding this comment.
This include is not needed because there are no uses that require the definition of Thread.
Right, seems like the forward declaration used to be provided by memory/allocation.hpp. Let's get rid of the include and use a forward declaration of our own instead.
There was a problem hiding this comment.
Should I open another PR to remove it? I can also remove the locking related to NMT in arena.cpp in the same PR as well (as per this comment).
There was a problem hiding this comment.
I've fixed that particular bug and integrated it already :-).
Summary
This PR just replaces
ThreadCriticalwith a lock specific to NMT.ThreadCriticalis a big lock and is unnecessary for the purposes of NMT. I've implemented the new lock with a semaphore so that it can be used early before VM init. There is also the possibility of adding assertions in places we expect NMT to have synchronization. I haven't added assertions yet in many places because some code paths such as the (NMT tests) don't lock yet. I think it makes sense to close any gaps in locking in another PR in which I can also add more assertions.Testing:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20852/head:pull/20852$ git checkout pull/20852Update a local copy of the PR:
$ git checkout pull/20852$ git pull https://git.openjdk.org/jdk.git pull/20852/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20852View PR using the GUI difftool:
$ git pr show -t 20852Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20852.diff
Webrev
Link to Webrev Comment