Skip to content

runtime: add async preemption support on riscv64 #38146

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

runtime: add async preemption support on riscv64 #38146

wants to merge 3 commits into from

Conversation

NonerKao
Copy link
Contributor

This CL adds support of call injection and async preemption on
riscv64. We also clobbered REG_TMP for the injected call. Unsafe
points related to REG_TMP access have been marked in previous commits.

Fixes #36711.

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Mar 29, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 08ba718) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226206 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Joel Sing:

Patch Set 1:

(6 comments)

Thanks for working on this - a quick first pass.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 788ef99) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226206 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Quey-Liang Kao:

Patch Set 1:

(6 comments)

Patch Set 1:

(6 comments)

Thanks for working on this - a quick first pass.

Thanks! BTW I have two newbie questions:

  1. Can I just force push the update? I already did on github though.
  2. How to use git codereview to initiate multi-commit patch review process? The contribution guide is not clear with this subject.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 2: Run-TryBot+1

(3 comments)

Thanks for doing this.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8e0c462d

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 2:

  1. How to use git codereview to initiate multi-commit patch review process? The contribution guide is not clear with this subject.

This is mostly documented at https://godoc.org/golang.org/x/review/git-codereview#hdr-Multiple_Commit_Work_Branches

Basically,

git codereview change
... work ...
git commit # as usual
... work ...
git commit
...
git codereview mail

If you need to rebase or adjust commits, you may find "git codereview sync" and "git codereview rebase-work" useful.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 2:

  1. Can I just force push the update? I already did on github though.

I'm not familiar with GitHub and Pull Request enough to answer this. In the end, if nothing works, you can just discard this and send a new CL or PR.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 2: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Quey-Liang Kao:

Patch Set 2:

(4 comments)

Patch Set 2: Run-TryBot+1

(3 comments)

Thanks for doing this.

Thanks for applying trybot. I myself run the test on Fedora/riscv on qemu, with
runtime/testdata/testprog/testprog AsyncPreempt
only, and it can show "OK" properly.

As a side note, on the same environment, I found that

  1. I cannot successfully bootstrap riscv64/Linux itself.
    2, The all.bash thus cannot produce stable results.
    Are they known issues or do you use other test environment?

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 03a06c4) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226206 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Joel Sing:

Patch Set 3:

(4 comments)

Patch Set 2:

(4 comments)

Patch Set 2: Run-TryBot+1

(3 comments)

Thanks for doing this.

Thanks for applying trybot. I myself run the test on Fedora/riscv on qemu, with
runtime/testdata/testprog/testprog AsyncPreempt
only, and it can show "OK" properly.

The current diff fails for me during build - I don't have the panic handy, but can provide it later if need be.

As a side note, on the same environment, I found that

  1. I cannot successfully bootstrap riscv64/Linux itself.
    2, The all.bash thus cannot produce stable results.
    Are they known issues or do you use other test environment?

The linux/riscv64 port is stable and ./all.bash passes (aside from one recently broken cmd/compile test) - I've done riscv64 work under qemu in the past, but I'm currently running fedora on a SiFive HiFive Unleashed board.

You'll want to check that the version of qemu is fairly current, as there was an atomics issue that was present (and is now fixed upstream). I've also heard of other issues that people have seen with more recent guest kernels.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Joel Sing:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

NonerKao added 3 commits April 15, 2020 20:31
@NonerKao
cmd/internal/obj/riscv: mark unsafe points
9e4a573 
For async preemption, we will be using REGTMP as a temporary
register in injected call on riscv64, which will clobber it.
So mark all codes that use REGTMP.

Change-Id: I4e32377427cb0f7360b3a91ed9e8b0224599a872
@NonerKao
cmd/internal/obj/riscv: refine spadj field
f8eb7a6 
The preprocess function calculates Spadj for each Prog purely based on
the stacksize of the function, which ignores frameless calls.  However,
if some frameless assembly function moves sp, the offset will not be
recorded correctly in pcln table, and thus it fails when executing
anything involving backtrace.

This is essential for async preempt support, because the injected call
asyncPreempt performs context save/restore on the stack.  It cannot
survive a runtime.GC() without correct sp information.

Change-Id: If5e3d9d57810b4fcdb538bda26848e088654ff80
@NonerKao
runtime: add async preemption support on riscv64
f6110d4 
This CL adds support of call injection and async preemption on
riscv64. We also clobbered REG_TMP for the injected call. Unsafe
points related to REG_TMP access have been marked in previous commits.

Fixes #36711.

Change-Id: I69d7f2ac5fde2279f62046725adbac69f98dd9ab
@gopherbot
Copy link
Contributor

Message from Quey-Liang Kao:

Patch Set 3:

(4 comments)

Patch Set 3:

(4 comments)

Patch Set 2:

(4 comments)

Patch Set 2: Run-TryBot+1

(3 comments)

Thanks for doing this.

Thanks for applying trybot. I myself run the test on Fedora/riscv on qemu, with
runtime/testdata/testprog/testprog AsyncPreempt
only, and it can show "OK" properly.

The current diff fails for me during build - I don't have the panic handy, but can provide it later if need be.

As a side note, on the same environment, I found that

  1. I cannot successfully bootstrap riscv64/Linux itself.
    2, The all.bash thus cannot produce stable results.
    Are they known issues or do you use other test environment?

The linux/riscv64 port is stable and ./all.bash passes (aside from one recently broken cmd/compile test) - I've done riscv64 work under qemu in the past, but I'm currently running fedora on a SiFive HiFive Unleashed board.

You'll want to check that the version of qemu is fairly current, as there was an atomics issue that was present (and is now fixed upstream). I've also heard of other issues that people have seen with more recent guest kernels.

Thanks for the hint. I can now bootstrap native go toolchain on qemu upstream.
Patchset 4 will be ready soon.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: f6110d4) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226206 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 4: Run-TryBot+1 Code-Review+1

(1 comment)

LGTM. If Joel could confirm that it passes all.bash, I think it is good to go. Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=03e36b95

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Joel Sing:

Patch Set 4: Code-Review+2

Patch Set 4: Run-TryBot+1 Code-Review+1

(1 comment)

LGTM. If Joel could confirm that it passes all.bash, I think it is good to go. Thanks!

Confirmed all.bash passes successfully.

Please don’t reply on this GitHub thread. Visit golang.org/cl/226206.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Apr 16, 2020
@NonerKao @4a6f656c
runtime: add async preemption support on riscv64
b89f4c6 
This CL adds support of call injection and async preemption on
riscv64. We also clobbered REG_TMP for the injected call. Unsafe
points related to REG_TMP access have been marked in previous commits.

Fixes #36711.

Change-Id: I1a1df5b7fc23eaafc34a6a6448fcc3c91054496e
GitHub-Last-Rev: f6110d4
GitHub-Pull-Request: #38146
Reviewed-on: https://go-review.googlesource.com/c/go/+/226206
Reviewed-by: Cherry Zhang <[email protected]>
Reviewed-by: Joel Sing <[email protected]>
Run-TryBot: Cherry Zhang <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/226206 has been merged.

@gopherbot gopherbot closed this Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

runtime: add async preemption support to the riscv port
3 participants
@NonerKao @gopherbot @googlebot