Skip to content
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.

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

8254264: Remove redundant cross_modify_fence() #655

Closed
wants to merge 3 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Oct 14, 2020

Hi all,

Please review the following patch that removes some uneeded uses of cross_modify_fence() in common code, in particular from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and java_suspend_self_with_safepoint_check(). These fences were added before each JavaThread had to disarm itself (8230594). After a safepoint/handshake each JavaThread will always call SafepointMechanism::process_if_requested_slow() when transitioning out of the safe state and will execute a cross_modify_fence().
Tested with mach5 tiers1-7.

Thanks,
Patricio


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) (3/9 running) (9/9 running) (1/9 running)

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/655/head:pull/655
$ git checkout pull/655

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 14, 2020

👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2020

@pchilano The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@pchilano pchilano marked this pull request as ready for review Oct 14, 2020
@openjdk openjdk bot added the rfr label Oct 14, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2020

Webrevs

@pchilano pchilano mentioned this pull request Oct 14, 2020
3 tasks done
@robehn
robehn approved these changes Oct 15, 2020
Copy link
Contributor

@robehn robehn left a comment

Thanks!

@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2020

@pchilano 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:

8254264: Remove redundant cross_modify_fence()

Reviewed-by: rehn, dholmes, dcubed

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 2 new commits pushed to the master branch:

  • 44f9271: 8254974: Fix stutter typo in TypeElement
  • 76fdd7f: 8255038: Adjust adapter_code_size to account for -Xlog:methodhandles in debug builds

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 15, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 15, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Patricio,

On 15/10/2020 1:20 am, Patricio Chilano Mateo wrote:

Hi all,

Please review the following patch that removes some uneeded uses of cross_modify_fence() in common code, in particular
from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and java_suspend_self_with_safepoint_check(). These fences
were added before each JavaThread had to disarm itself (8230594). After a safepoint/handshake each JavaThread will
always call SafepointMechanism::process_if_requested_slow() when transitioning out of the safe state and will execute a
cross_modify_fence(). Tested with mach5 tiers1-7.

As you have already done the analysis, could you show the exact call
sequences that allow for the removal of the code as described? I'm
trying to verify what you've stated but trying to reverse engineer the
calling sequences is painful. :)

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 15, 2020

Mailing list message from Patricio Chilano on hotspot-runtime-dev:

Hi David,

On 10/15/20 5:22 AM, David Holmes wrote:

Hi Patricio,

On 15/10/2020 1:20 am, Patricio Chilano Mateo wrote:

Hi all,

Please review the following patch that removes some uneeded uses of
cross_modify_fence() in common code, in particular
from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and
java_suspend_self_with_safepoint_check(). These fences
were added before each JavaThread had to disarm itself (8230594).
After a safepoint/handshake each JavaThread will
always call SafepointMechanism::process_if_requested_slow() when
transitioning out of the safe state and will execute a
cross_modify_fence(). Tested with mach5 tiers1-7.

As you have already done the analysis, could you show the exact call
sequences that allow for the removal of the code as described? I'm
trying to verify what you've stated but trying to reverse engineer the
calling sequences is painful. :)

So, before each JavaThread had to disarm itself you could have the
following sequence:

- JT T creates ThreadBlockInVM object and then blocks (usually in
park()/wait())
- VMThread arms T for a safepoint operation. Since T is in
_thread_blocked state it's safe so safepoint starts.
- Once safepoint operation is done VMThread disarms T
- T wakes up and executes ~ThreadBlockInVM(). Since the poll word/page
is not armed it just continues execution. If we didn't have that
cross_modify_fence() in ~ThreadBlockInVM() we can transition back to
java later without executing the fence instruction.

Same sequence with ~ThreadBlockInVMWithDeadlockCheck and
java_suspend_self_with_safepoint_check(). So when transitioning out of
the _thread_blocked state a JT wouldn't know if a safepoint happened so
we had that fence there unconditionally.

Today disarming of the poll word/page is done by each JT, so if a
safepoint happened while the JT was blocked the poll word/page will
remain armed. The JT when transitioning out of the _thread_blocked will
call SafepointMechanism::process_if_requested() (in
~TBIVM/~TBIVMWDC/java_suspend_self_with_safepoint_check()), will go
through process_if_requested_slow() to check if it needs to process some
operation and at the end will execute a cross_modify_fence().

Patricio

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 16, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Patricio,

On 16/10/2020 4:09 am, Patricio Chilano wrote:

Hi David,

On 10/15/20 5:22 AM, David Holmes wrote:

Hi Patricio,

On 15/10/2020 1:20 am, Patricio Chilano Mateo wrote:

Hi all,

Please review the following patch that removes some uneeded uses of
cross_modify_fence() in common code, in particular
from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and
java_suspend_self_with_safepoint_check(). These fences
were added before each JavaThread had to disarm itself (8230594).
After a safepoint/handshake each JavaThread will
always call SafepointMechanism::process_if_requested_slow() when
transitioning out of the safe state and will execute a
cross_modify_fence(). Tested with mach5 tiers1-7.

As you have already done the analysis, could you show the exact call
sequences that allow for the removal of the code as described? I'm
trying to verify what you've stated but trying to reverse engineer the
calling sequences is painful. :)
So, before each JavaThread had to disarm itself you could have the
following sequence:

- JT T creates ThreadBlockInVM object and then blocks (usually in
park()/wait())
- VMThread arms T for a safepoint operation. Since T is in
_thread_blocked state it's safe so safepoint starts.
- Once safepoint operation is done VMThread disarms T
- T wakes up and executes ~ThreadBlockInVM(). Since the poll word/page
is not armed it just continues execution. If we didn't have that
cross_modify_fence() in ~ThreadBlockInVM() we can transition back to
java later without executing the fence instruction.

And just to be clear any action that changes code such that a
cross-modify-fence is needed by other threads, has to perform a global
safepoint/handshake?

Same sequence with ~ThreadBlockInVMWithDeadlockCheck and
java_suspend_self_with_safepoint_check(). So when transitioning out of
the _thread_blocked state a JT wouldn't know if a safepoint happened so
we had that fence there unconditionally.

Today disarming of the poll word/page is done by each JT, so if a
safepoint happened while the JT was blocked the poll word/page will
remain armed. The JT when transitioning out of the _thread_blocked will
call SafepointMechanism::process_if_requested() (in
~TBIVM/~TBIVMWDC/java_suspend_self_with_safepoint_check()), will go
through process_if_requested_slow() to check if it needs to process some
operation and at the end will execute a cross_modify_fence().

Okay so ...

~ThreadBlockInVM()
-> trans(_thread_blocked, _thread_in_vm);
--> transition(...);
---> SafepointMechanism::process_if_requested(thread)
-----> if (!local_poll_armed(thread)) { // <- It is armed
return;
}
process_if_requested_slow(thread); => OK

~ThreadBlockInVMWithDeadlockCheck()
-> if (SafepointMechanism::should_process(_thread)) { // True as poll armed
release_mutex();
SafepointMechanism::process_if_requested(_thread);
------> if (!local_poll_armed(thread)) { // <- It is armed
return;
}
process_if_requested_slow(thread); => OK

JavaThread::java_suspend_self_with_safepoint_check()
-> if (state != _thread_in_native) {
SafepointMechanism::process_if_requested(this); => OK
}

Those cases are all good as you stated.

So my only uncertainty now is the _thread_in_native case for suspension.
But how can we be _thread_in_native at the time we call
java_suspend_self_with_safepoint_check? When we call from
check_safepoint_and_suspend_for_native_trans we are in
_thread_in_native_trans. So that leaves
handle_special_runtime_exit_condition, which is called from:

- JavaCallWrapper::JavaCallWrapper
- thread must be _thread_in_vm here

- SafepointSynchronize::block
- can't be _thread_in_native (fatal error)

- ~ThreadInVMfromJava()
- thread must be _thread_in_Java

- ThreadToNativeFromVM
- thread must be _thread_in_vm

- ~ThreadInVMfromJavaNoAsyncException
- thread must be _thread_in_vm

So as far as I can see we can never be _thread_in_native when calling
JavaThread::java_suspend_self_with_safepoint_check. So it seems I don't
have to worry about that case after all :) (and in any case when leaving
_thread_in_native we will call
check_safepoint_and_suspend_for_native_trans so we'd get to the
cross-modify-fence via that route.)

So long story short: this all looks good to me. :)

Thanks,
David
-----

Patricio

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 16, 2020

Mailing list message from Patricio Chilano on hotspot-runtime-dev:

Hi David,

On 10/16/20 1:58 AM, David Holmes wrote:

Hi Patricio,

On 16/10/2020 4:09 am, Patricio Chilano wrote:

Hi David,

On 10/15/20 5:22 AM, David Holmes wrote:

Hi Patricio,

On 15/10/2020 1:20 am, Patricio Chilano Mateo wrote:

Hi all,

Please review the following patch that removes some uneeded uses of
cross_modify_fence() in common code, in particular
from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and
java_suspend_self_with_safepoint_check(). These fences
were added before each JavaThread had to disarm itself (8230594).
After a safepoint/handshake each JavaThread will
always call SafepointMechanism::process_if_requested_slow() when
transitioning out of the safe state and will execute a
cross_modify_fence(). Tested with mach5 tiers1-7.

As you have already done the analysis, could you show the exact call
sequences that allow for the removal of the code as described? I'm
trying to verify what you've stated but trying to reverse engineer
the calling sequences is painful. :)
So, before each JavaThread had to disarm itself you could have the
following sequence:

- JT T creates ThreadBlockInVM object and then blocks (usually in
park()/wait())
- VMThread arms T for a safepoint operation. Since T is in
_thread_blocked state it's safe so safepoint starts.
- Once safepoint operation is done VMThread disarms T
- T wakes up and executes ~ThreadBlockInVM(). Since the poll
word/page is not armed it just continues execution. If we didn't have
that cross_modify_fence() in ~ThreadBlockInVM() we can transition
back to java later without executing the fence instruction.

And just to be clear any action that changes code such that a
cross-modify-fence is needed by other threads, has to perform a global
safepoint/handshake?

I'm not sure of that. But we added these fences for the case where the
code got patched during a safepoint. Maybe Robbin knows the answer.? : )

Same sequence with ~ThreadBlockInVMWithDeadlockCheck and
java_suspend_self_with_safepoint_check(). So when transitioning out
of the _thread_blocked state a JT wouldn't know if a safepoint
happened so we had that fence there unconditionally.

Today disarming of the poll word/page is done by each JT, so if a
safepoint happened while the JT was blocked the poll word/page will
remain armed. The JT when transitioning out of the _thread_blocked
will call SafepointMechanism::process_if_requested() (in
~TBIVM/~TBIVMWDC/java_suspend_self_with_safepoint_check()), will go
through process_if_requested_slow() to check if it needs to process
some operation and at the end will execute a cross_modify_fence().

Okay so ...

~ThreadBlockInVM()
-> trans(_thread_blocked, _thread_in_vm);
--> transition(...);
---> SafepointMechanism::process_if_requested(thread)
-----> if (!local_poll_armed(thread)) { // <- It is armed
???????? return;
?????? }
?????? process_if_requested_slow(thread); => OK

~ThreadBlockInVMWithDeadlockCheck()
-> if (SafepointMechanism::should_process(_thread)) {? // True as poll
armed
???? release_mutex();
???? SafepointMechanism::process_if_requested(_thread);
------> if (!local_poll_armed(thread)) { // <- It is armed
????????? return;
??????? }
??????? process_if_requested_slow(thread); => OK

JavaThread::java_suspend_self_with_safepoint_check()
-> if (state != _thread_in_native) {
??? SafepointMechanism::process_if_requested(this); => OK
? }

Those cases are all good as you stated.

So my only uncertainty now is the _thread_in_native case for
suspension. But how can we be _thread_in_native at the time we call
java_suspend_self_with_safepoint_check? When we call from
check_safepoint_and_suspend_for_native_trans we are in
_thread_in_native_trans. So that leaves
handle_special_runtime_exit_condition, which is called from:

- JavaCallWrapper::JavaCallWrapper
? - thread must be _thread_in_vm here

- SafepointSynchronize::block
? - can't be _thread_in_native (fatal error)

- ~ThreadInVMfromJava()
?- thread must be _thread_in_Java

- ThreadToNativeFromVM
? - thread must be _thread_in_vm

- ~ThreadInVMfromJavaNoAsyncException
? - thread must be _thread_in_vm

So as far as I can see we can never be _thread_in_native when calling
JavaThread::java_suspend_self_with_safepoint_check.

Only with ThreadToNativeFromVM, because we switch to native and then
call has_special_runtime_exit_condition(). Which by the way, I don't see
why we need to make that call since we are going from vm->native. We
don't deliver async exceptions and we don't need to check for suspend
since we are going to native. If we remove that call we can remove that
conditional in java_suspend_self_with_safepoint_check() and always call
SafepointMechanism::process_if_requested().

So it seems I don't have to worry about that case after all :) (and in
any case when leaving _thread_in_native we will call
check_safepoint_and_suspend_for_native_trans so we'd get to the
cross-modify-fence via that route.)

Exactly.

\So long story short: this all looks good to me. :)

Thanks David!

Patricio

Thanks,
David
-----

Patricio

Thanks,
David

@robehn
Copy link
Contributor

@robehn robehn commented Oct 16, 2020

And just to be clear any action that changes code such that a
cross-modify-fence is needed by other threads, has to perform a global
safepoint/handshake?

I'm not sure of that. But we added these fences for the case where the
code got patched during a safepoint. Maybe Robbin knows the answer.? : )

On x86 we have two compliant option according to the manual:

(* OPTION 1 *)
Store modified code (as data) into code segment;
Jump to new code or an intermediate location;
Execute new code;

(* OPTION 2 *)
Store modified code (as data) into code segment;
Execute a serializing instruction; (* For example, CPUID instruction *)
Execute new code;
  • Code patching that happens outside a safepoint do options 1 and don't need CMF.
    If CMF is needed on some arch, it must be encoded into the instruction stream while patching.
    (whether this is really safe, thinking of spectre, is another story :) )
    See e.g. NativeGeneralJump::replace_mt_safe()
  • Code patching that happens inside a safepoint do options 2 and need CMF.
    We actual only need CMF for safepoints, but since we do not know why we were armed we must emit it.
    So can be optimize by keeping track of the safepoint id and for which safepoint id we have emitted CMF for per java thread.
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 18, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

<trimming>

On 17/10/2020 4:33 am, Patricio Chilano wrote:

On 10/16/20 1:58 AM, David Holmes wrote:

So my only uncertainty now is the _thread_in_native case for
suspension. But how can we be _thread_in_native at the time we call
java_suspend_self_with_safepoint_check? When we call from
check_safepoint_and_suspend_for_native_trans we are in
_thread_in_native_trans. So that leaves
handle_special_runtime_exit_condition, which is called from:

- JavaCallWrapper::JavaCallWrapper
? - thread must be _thread_in_vm here

- SafepointSynchronize::block
? - can't be _thread_in_native (fatal error)

- ~ThreadInVMfromJava()
?- thread must be _thread_in_Java

- ThreadToNativeFromVM
? - thread must be _thread_in_vm

- ~ThreadInVMfromJavaNoAsyncException
? - thread must be _thread_in_vm

So as far as I can see we can never be _thread_in_native when calling
JavaThread::java_suspend_self_with_safepoint_check.
Only with ThreadToNativeFromVM, because we switch to native and then
call has_special_runtime_exit_condition().

Doh! Yes that was a silly oversight on my part.

Thanks,
David
-----

Which by the way, I don't see

why we need to make that call since we are going from vm->native. We
don't deliver async exceptions and we don't need to check for suspend
since we are going to native. If we remove that call we can remove that
conditional in java_suspend_self_with_safepoint_check() and always call
SafepointMechanism::process_if_requested().

So it seems I don't have to worry about that case after all :) (and in
any case when leaving _thread_in_native we will call
check_safepoint_and_suspend_for_native_trans so we'd get to the
cross-modify-fence via that route.)
Exactly.

\So long story short: this all looks good to me. :)
Thanks David!

Patricio

Thanks,
David
-----

Patricio

Thanks,
David

@a74nh
Copy link
Contributor

@a74nh a74nh commented Oct 19, 2020

I merged this patch together with #428 (which makes AArch64 use cross modify fence) and ran it on AArch64 with the "-XX:+VerifyCrossModifyFence" set. A complete tier1 ran successfully, and I'm waiting on the results of a much wider run.

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 19, 2020

Thanks @robehn and @dholmes-ora for the reviews!

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 19, 2020

I merged this patch together with #428 (which makes AArch64 use cross modify fence) and ran it on AArch64 with the "-XX:+VerifyCrossModifyFence" set. A complete tier1 ran successfully, and I'm waiting on the results of a much wider run.

Ok, let me know when you are okay and I will integrate.

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Oct 19, 2020

Heads-up: The build and test tasks appear to be unhappy.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up! Had to read through the comments a couple of
times along with the code before I convinced myself all was
okay. Thanks for testing Mach5 Tier[1-7].

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 19, 2020

Heads-up: The build and test tasks appear to be unhappy.

Yes, commit v1 was done after 8173585 was pushed but before the issues introduced by it were fixed. I can merge with master to make it happy. : )

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 19, 2020

Thumbs up! Had to read through the comments a couple of
times along with the code before I convinced myself all was
okay. Thanks for testing Mach5 Tier[1-7].

Thanks Dan!

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Oct 19, 2020

Why is the commit history only showing a single commit if there was a merge in the past?
(I might be missing something trivial here since I'm still a GitHub newbie.)

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 19, 2020

Why is the commit history only showing a single commit if there was a merge in the past?
(I might be missing something trivial here since I'm still a GitHub newbie.)

Not sure, where are you seeing the merge?

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Oct 20, 2020

Sorry Patricio. I saw the comment from a74nh above ("I merged this patch together with...")
and I just assumed it was you replying to David H. My mistake.

@a74nh
Copy link
Contributor

@a74nh a74nh commented Oct 20, 2020

I merged this patch together with #428 (which makes AArch64 use cross modify fence) and ran it on AArch64 with the "-XX:+VerifyCrossModifyFence" set. A complete tier1 ran successfully, and I'm waiting on the results of a much wider run.

Ok, let me know when you are okay and I will integrate.

I've run this across all our tests and machines now, and everything passes ok.

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Oct 20, 2020

/integrate

@openjdk openjdk bot closed this Oct 20, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@pchilano Since your change was applied there have been 2 commits pushed to the master branch:

  • 44f9271: 8254974: Fix stutter typo in TypeElement
  • 76fdd7f: 8255038: Adjust adapter_code_size to account for -Xlog:methodhandles in debug builds

Your commit was automatically rebased without conflicts.

Pushed as commit f167a71.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@pchilano pchilano deleted the pchilano:8254264-cross_modify_fence branch Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants