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

Jump to bottom

runtime: support preemption on windows/arm and windows/arm64 #49759

Closed
zx2c4 opened this issue Nov 23, 2021 · 9 comments
Closed

runtime: support preemption on windows/arm and windows/arm64 #49759

zx2c4 opened this issue Nov 23, 2021 · 9 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 23, 2021 •
edited

There's this old CL that needs a rebase (CL 207961) and recently there was a discussion in #49458 about a bug stemming from the lack of preemption on windows/arm64.

We should implement preemption on windows/arm and windows/arm64. This issue is to track that work.

CC @bufflig @aclements @cherrymui @mknyszek @bcmills @rsc
@gopherbot
Copy link

Change https://golang.org/cl/366734 mentions this issue: runtime: support non-cooperative preemption on windows/arm

@gopherbot
Copy link

Change https://golang.org/cl/366735 mentions this issue: runtime: support non-cooperative preemption on windows/arm64

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 23, 2021

Turned out to be pretty quick to implement. Trybots passed for the arm64 commit. We're now waiting for #47019 to land so that we can run the trybots on arm32 too. Then we'll have preemption on Windows on all platforms and can merge those two CLs.

CC @toothrot @dmitshur @heschi

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 23, 2021

@rsc - do you have opinions on this with regards to 1.18 vs 1.19?

@bcmills
Copy link
Contributor

bcmills commented Nov 23, 2021

(Just FYI: I don't think Russ receives GitHub notifications; freeze exceptions are in general up to the release team to decide.)

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 23, 2021

Oh, okay, this is freeze thing, not general Go planning, makes sense.

@dmitshur - up to you then.

@dmitshur
Copy link
Contributor

@zx2c4 We've discussed this on the release team, and in general we are okay with this happening for Go 1.18, especially if it can help fix other bugs. It seems the CLs are reviewed and ready to go, so if submitted before Beta 1 they'll undergo all the testing it receives. If it causes new problems, I hope it'll be possible to attribute it back to this change and revert it (the change is very small and so doesn't pose a significant risk of conflicts).

Please note that #47019 is not resolved yet, and though we're hoping to make progress on it (latest update on it is from yesterday), it's hard to say exactly how soon it will be resolved. Perhaps you'd prefer to land this change for the windows/arm64 port independently of the windows/arm one, unless you think it is better to bundle them and land at the same time.

We are in the part of the release process where the goal is to fix bugs and increase stability/reliability of the upcoming release, so if you think this can be integrated safely for 1.18, please proceed. Otherwise it will be safe to target this for 1.19 during its development cycle. Thank you Jason.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 24, 2021

@dmitshur Rather than waiting for #47019, for this particular CL, I'll just run all.bat on my own ARM machine at home and report the result back to the CL.

gopherbot pushed a commit that referenced this issue Nov 25, 2021
@zx2c4
runtime: support non-cooperative preemption on windows/arm
c58243a 
This adds support for injecting asynchronous preemption calls on
windows/arm. This code follows sigctxt.pushCall for POSIX OSes
on arm, except we subtract 1 from IP, just as in CL 273727.

Updates #10958.
Updates #24543.
Updates #49759.

Change-Id: Id0c2aed28662f50631b8c8cede3b4e6f088dafea
Reviewed-on: https://go-review.googlesource.com/c/go/+/366734
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Patrik Nyblom <pnyb@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@dmitshur dmitshur added this to the Go1.18 milestone Nov 25, 2021
@dmitshur dmitshur added FeatureRequest NeedsFix The path to resolution is known, but the work has not been done. labels Nov 25, 2021
@gopherbot
Copy link

Change https://golang.org/cl/367095 mentions this issue: doc/go1.18: document non-cooperative preemption on windows/arm{,64}

gopherbot pushed a commit that referenced this issue Nov 26, 2021
@zx2c4
doc/go1.18: document non-cooperative preemption on windows/arm{,64}
7703804 
For #47694.
Updates #49759.

Change-Id: I7accd81b8ea6c31e4a2b5e155cf93fe9c447813b
Reviewed-on: https://go-review.googlesource.com/c/go/+/367095
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@golang golang locked and limited conversation to collaborators Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
4 participants
@zx2c4 @dmitshur @bcmills @gopherbot