Skip to content

8334890: Missing unconditional cross modifying fence in nmethod entry barriers #19990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fisk wants to merge 1 commit into from
Closed

8334890: Missing unconditional cross modifying fence in nmethod entry barriers #19990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions src/hotspot/share/gc/shared/barrierSetNMethod.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,16 +178,9 @@ int BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr) {
nmethod* nm = cb->as_nmethod();
BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod();

// Check for disarmed method here to avoid going into DeoptimizeNMethodBarriersALot code
// too often. nmethod_entry_barrier checks for disarmed status itself,
// but we have no visibility into whether the barrier acted or not.
if (!bs_nm->is_armed(nm)) {
return 0;
}

assert(!nm->is_osr_method(), "Should not reach here");
// Called upon first entry after being armed
bool may_enter = bs_nm->nmethod_entry_barrier(nm);
assert(!nm->is_osr_method() || may_enter, "OSR nmethods should always be entrant after migration");

// In case a concurrent thread disarmed the nmethod, we need to ensure the new instructions
// are made visible, by using a cross modify fence. Note that this is synchronous cross modifying
Expand All @@ -197,11 +190,11 @@ int BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr) {
// it can be made conditional on the nmethod_patching_type.
OrderAccess::cross_modify_fence();

// 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()) {
Copy link

@kimbarrett kimbarrett Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static volatile uint32_t counter=0;
if (Atomic::add(&counter, 1u) % 3 == 0) {
if (Atomic::add(&counter, 1u) % 10 == 0) {
Comment on lines -200 to +197
Copy link
Member

@xmas92 xmas92 Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

may_enter = false;
}
}
Expand All @@ -214,15 +207,6 @@ int BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr) {
}

bool BarrierSetNMethod::nmethod_osr_entry_barrier(nmethod* nm) {
// This check depends on the invariant that all nmethods that are deoptimized / made not entrant
// are NOT disarmed.
// This invariant is important because a method can be deoptimized after the method have been
// resolved / looked up by OSR by another thread. By not deoptimizing them we guarantee that
// a deoptimized method will always hit the barrier and come to the same conclusion - deoptimize
if (!is_armed(nm)) {
return true;
}

assert(nm->is_osr_method(), "Should not reach here");
log_trace(nmethod, barrier)("Running osr nmethod entry barrier: " PTR_FORMAT, p2i(nm));
bool result = nmethod_entry_barrier(nm);
Expand Down