8358821: patch_verified_entry causes problems, use nmethod entry barriers instead #25764
Conversation
|
👋 Welcome back dlong! A progress list of the required criteria for merging this PR into |
|
@dean-long 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 94 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 |
|
@dean-long The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
hotspot
Webrevs
|
|
/label hotspot-gc hotspot-runtime |
openjdk
bot
added
hotspot-gc
|
@dean-long The |
There was a problem hiding this comment.
I appreciate this PR. I like using the nmethod entry barrier and getting rid of not-entrant patching.
I have some change requests. In addition, we can get rid of much more code. I've put my proposal in a Commit: TheRealMDoerr@4aed569
Please take a look!
|
/contributor add @TheRealMDoerr |
|
@dean-long Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
Thanks @TheRealMDoer for the suggestions, I incorporated all of them, however for shenandoahCodeRoots.cpp I decided to make arm() public, so the caller code doesn't need to know about the magic value 0. |
Excellent! Thank you! I'll take another look over it and run tests. |
|
@dean-long |
There was a problem hiding this comment.
LGTM, now. I think is_sigill_not_entrant should also be removed completely: TheRealMDoerr@f61f54a
I've run tests on some platforms, but will run on more.
@offamitkumar, @RealFYang, @bulasevich: You may also want to check your platforms.
@fisk: You may want to take a look at the nmethod entry barrier changes.
| ZLocker<ZReentrantLock> locker(ZNMethod::lock_for_nmethod(nm)); | ||
| // Preserve the sticky bit | ||
| if (is_not_entrant(nm)) { | ||
| value |= not_entrant; |
There was a problem hiding this comment.
Is it possible to have a race where another thread sets an nmethod to not entrant and the thread calling this making the nmethod entry barrier not entrant?
If this was called to disarm a method and then enter it, it seems a bit sneaky in that case that we pass the nmethod entry barrier even though we under the lock see that it is not entrant. Probably okay but still feels like it might be more robust if the thread setting an nmethod to not entrant is always the one that arms the nmethod entry barrier.
There was a problem hiding this comment.
If I understand your concern correctly, there is no race. The only caller of BarrierSetNMethod::make_not_entrant() is nmethod::make_not_entrant(), and it is done inside a NMethodState_lock critical section. After a call to nmethod::make_not_entrant(), the nmethod entry barrier is armed and stays that way.
And by design, a disarm only disarms at the inner nmethod_entry_barrier level, not the outer nmethod_stub_entry_barrier level.
There was a problem hiding this comment.
My concern is that while thread 1 calls nmethod::make_not_entrant(), thread 2 racingly performs nmethod entry barrier; it makes the is_not_entrant check before it gets updated, but then it gets updated as the per nmethod lock is taken. The GC code "disarms" the GC barrier but in doing so finds that "oh this should be not entrant", but that's sort of not reflected as thread 2 will then proceed with entering the nmethod it just armed as not entrant in the nmethod entry barrier code.
Does that make sense?
|
Just FYI, s390 build is broken with this change: stack trace: |
|
@offamitkumar: The problem is probably the initialization to -1: |
|
Seems like arm32 has the same issue: The init value shouldn't have the sticky bit set. |
Thank you Martin for the suggestion. @dean-long would you please add this diff, fixing s390x build. I ran tier1 test with fastdebug, test are clean; diff --git a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
index e78906708af..2d663061aec 100644
--- a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
+++ b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
@@ -180,7 +180,7 @@ void BarrierSetAssembler::nmethod_entry_barrier(MacroAssembler* masm) {
__ z_lg(Z_R0_scratch, in_bytes(bs_nm->thread_disarmed_guard_value_offset()), Z_thread); // 6 bytes
// Compare to current patched value:
- __ z_cfi(Z_R0_scratch, /* to be patched */ -1); // 6 bytes (2 + 4 byte imm val)
+ __ z_cfi(Z_R0_scratch, /* to be patched */ 0); // 6 bytes (2 + 4 byte imm val)
// Conditional Jump
__ z_larl(Z_R14, (Assembler::instr_len((unsigned long)LARL_ZOPC) + Assembler::instr_len((unsigned long)BCR_ZOPC)) / 2); // 6 bytes
diff --git a/src/hotspot/cpu/s390/stubGenerator_s390.cpp b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
index d3f6540a3ea..bb1d9ce6037 100644
--- a/src/hotspot/cpu/s390/stubGenerator_s390.cpp
+++ b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
@@ -3197,7 +3197,7 @@ class StubGenerator: public StubCodeGenerator {
// VM-Call: BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr)
__ call_VM_leaf(CAST_FROM_FN_PTR(address, BarrierSetNMethod::nmethod_stub_entry_barrier));
- __ z_ltr(Z_R0_scratch, Z_RET);
+ __ z_ltr(Z_RET, Z_RET);
// VM-Call Epilogue
__ restore_volatile_regs(Z_SP, frame::z_abi_160_size, true, false); |
|
/contributor add @offamitkumar |
|
@dean-long |
|
Thanks @offamitkumar. Could you explain the |
Thanks, I pushed a potential fix for that. |
For ZGC I am using a per-nmethod lock: I don't know what benchmarks to run to check the performance for functions like Deoptimization::deoptimize_all_marked, so I welcome any help with this. One possible optimization that might help is skipping the lock if the make_not_entrant call is done during a safepoint. |
|
Unfortunately, 0xBEAFDEAD also has the MSB set. Shouldn't we better use 0 like on all other platforms? |
Oops, I was tripped up trying to be clever. Yes, I'm fine with using 0. I'll fix it. |
This PR removes patching of the verified entry point and related code, and replaces it by refactoring the existing nmethod entry barrier.
We used to patch the verified entry point to make sure it was not_entrant. The patched entry point then redirected to SharedRuntime::handle_wrong_method(), either directly with a jump to a stub, or indirectly with an illegal instruction and the help of the signal handler. The not_entrant state is a final state, so once an nmethod becomes not_entrant, it stays not_entrant. We can do the same thing with a permanently armed nmethod entry barrier.
The solution I went with reserves one bit of the entry barrier guard value. This bit must remain set, so I call it a "sticky" bit. Setting the guard value now is effectively like setting a bitfield, so I needed to add a lock around it. The alternative would be to change the platform-specific code to do compare-and-swap.
For the lock, I introduced a new NMethodEntryBarrier_lock, whose only purpose is to make the update to the guard value atomic. For ZGC, I decided to use the existing per-nmethod lock ZNMethod::lock_for_nmethod(). I suspect we could do the same for Shenandoah, if needed for performance.
This change also makes it a bit clearer that the nmethod entry barrier effectively has two levels. Level 0 is the outer level or layer controlled by BarrierSetNMethod::nmethod_stub_entry_barrier(), and the inner layer controlled by BarrierSetNMethod::nmethod_entry_barrier(). This could be generalized if we decide we need more flavors of entry barriers. The inner barrier is mostly ignorant of the fact that the outer guard is multiplexing for both levels.
Progress
Issue
Reviewers
Contributors
<mdoerr@openjdk.org><amitkumar@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25764/head:pull/25764$ git checkout pull/25764Update a local copy of the PR:
$ git checkout pull/25764$ git pull https://git.openjdk.org/jdk.git pull/25764/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25764View PR using the GUI difftool:
$ git pr show -t 25764Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25764.diff
Using Webrev
Link to Webrev Comment