8334890: Missing unconditional cross modifying fence in nmethod entry barriers #19990
Conversation
|
👋 Welcome back eosterlund! A progress list of the required criteria for merging this PR into |
|
@fisk 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 39 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. ➡️ To integrate this PR with the above commit message to the |
|
Performance results are neutral, as expected. Tier1-5 passed. |
| // Diagnostic option to force deoptimization 1 in 3 times. It is otherwise | ||
| // Diagnostic option to force deoptimization 1 in 10 times. It is otherwise | ||
| // a very rare event. | ||
| if (DeoptimizeNMethodBarriersALot) { | ||
| if (DeoptimizeNMethodBarriersALot && !nm->is_osr_method()) { | ||
| static volatile uint32_t counter=0; | ||
| if (Atomic::add(&counter, 1u) % 3 == 0) { | ||
| if (Atomic::add(&counter, 1u) % 10 == 0) { |
There was a problem hiding this comment.
I have not good intuition about the frequency of this, and how this affects things. So have hard time commenting on this change.
An alternative would be to just fence when the nmethod is already disarmed and not call bs_nm->nmethod_entry_barrier(nm);. This is what effectively already happens in all implementations of nmethod_entry_barrier after JDK-8331911 / #19285. (Maintaining the current behaviour with respect to DeoptimizeNMethodBarriersALot).
But as long as this new magic constant seems to have similar testing coverage when running our tests that use DeoptimizeNMethodBarriersALot this seems like a sensible solution as well.
| // a very rare event. | ||
| if (DeoptimizeNMethodBarriersALot) { | ||
| if (DeoptimizeNMethodBarriersALot && !nm->is_osr_method()) { |
There was a problem hiding this comment.
This could also include may_enter in the conditions, since the effect of this bit of code is to set it false.
But maybe that's a rare thing and not worth checking for here.
|
Thanks for the reviews @kimbarrett and @xmas92! |
|
Going to push as commit c0604fb.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
/backport :jdk23 |
|
@fisk Could not automatically backport
Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk. Note: these commands are just some suggestions and you can use other equivalent commands you know. Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk with the title Below you can find a suggestion for the pull request body:
|
|
I see you integrated; good job, and thank you to the Reviewers. The narrative at the top of this PR is excellent for motivating and explaining the removal of the extra check. Some of the other diffs are more mysterious, as Axel noted. There are no API docs in barrierSetNMethod.[ch]pp so I would need to trace through all the code paths to properly educate myself about the effect of this change. For example, BarrierSetNMethod::nmethod_entry_barrier is a very interesting function, along with its OSR brother, but there are no comments directly visible here that give a clue as to when it is called, or why it must be called. I think that level of non-documentation is often a maintenance problem. I see this file relates to the larger API in barrierSet.hpp but that file has sparse comments also. Ideally, I’d hope to read The Narrative of the Barrier Set at the top of barrierSet.hpp, and maybe have a brief pointer to The Narrative from less-commented related files like barrierSetNMethod.cpp. Also, if I did this change, and was feeling chatty and cautious, I’d leave behind an informative comment to the effect that “you might want to double-check the barrier state here, but don’t, because races”. It’s nice not to leave a seam from past history, but sometimes the absence of a warning leads people to repeat history. None of the above critiques would have stopped me from approving the change as another Reviewer, but the lack of documentation would have made me hesitate to review quickly. |
On x86_64, our nmethod entry barriers use a mix of asynchronous and synchronous code modification. There is a cmp instruction with an immediate. When the immediate value is "incorrect", the nmethod is armed, and when it's "correct", it's disarmed. When we load the immediate with the instruction fetcher, we use asynchronous cross modifying code, and when we load the immediate as data, we use synchronous cross modifying code.
We use asynchronous code modification in the fast path of nmethod entry barriers. If the nmethod is concurrently being disarmed while the nmethod entry barrier is executed, then we are guaranteed that if the updated "correct" immediate is observed by the instruction fetcher, then any code modification to the nmethod prior to disarming it on another thread, is guaranteed to also be observed by the instruction fetcher.
However, in the slow path, when the immediate was observed to have the "incorrect" value by the instruction fetcher, we call a C++ function, BarrierSetNMethod::nmethod_stub_entry_barrier. In this function we check if the nmethod is disarmed or armed, by loading the guard value (from the immediate), as data. If we observe the updated value, indicating that the nmethod has become disarmed, we want to enter the nmethod. However, since we used data to signal that the instruction cross modification has happened, it is not safe to execute the concurrently modified instructions, without enforcing a cross modifying code fence. This is synchronous code modification.
There is some questionable optimization that in the stub slow path entry (which we just got to because the nmethod was observed to be armed by the instruction fetcher). It checks "just one more time" if the nmethod concurrently got disarmed, and then exits without cross modification fence. This is an opportunistic optimization that is very unlikely to be useful, since we got into the slow path because it a couple of instructions ago was armed. This opportunistic optimization breaks the synchronous code modification contract, which is that you have to issue an instruction cross modification fence after reading the data that signalled that cross modification has completed successfully.
This patch removes these kinds of opportunistic optimizations from the nmethod entry barrier code, in order to make it more robust and follow the synchronous cross modification dance correctly.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19990/head:pull/19990$ git checkout pull/19990Update a local copy of the PR:
$ git checkout pull/19990$ git pull https://git.openjdk.org/jdk.git pull/19990/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19990View PR using the GUI difftool:
$ git pr show -t 19990Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19990.diff
Webrev
Link to Webrev Comment