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

runtime: add async preemption support to the riscv port #36711

Closed
4a6f656c opened this issue Jan 23, 2020 · 6 comments
Closed

runtime: add async preemption support to the riscv port #36711

4a6f656c opened this issue Jan 23, 2020 · 6 comments

Comments

@4a6f656c
Copy link
Contributor

@4a6f656c 4a6f656c commented Jan 23, 2020

The Go riscv64 port does not currently support async preemption - this should be added.

@toothrot toothrot added this to the Backlog milestone Jan 23, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Jan 23, 2020

@aclements, item for #36365 ?

@tklauser tklauser added the arch-riscv label Jan 27, 2020
@NonerKao
Copy link
Contributor

@NonerKao NonerKao commented Feb 29, 2020

It has been a month since this issue was reported. Is there any work in progress?

I am new here but I have experience tracing/tweaking RISC-V context handling in Linux kernel. Now PPC64 and MIPS* async-preemption support are fine references, so I'll start from their ports. Any other hints are welcomed.

@NonerKao
Copy link
Contributor

@NonerKao NonerKao commented Mar 28, 2020

Basically riscv64 can apply pushCall() from arm64 with slight changes and have a straightforward context saving/restoring runtime.asyncPreempt(). I then test this simple setup with src/runtime/testdata/testprog/preempt.go. Precisely speaking, I disable the dummy function test and the frameless test for simplicity, leaving only the infinite loop for this test.

The infinite loop can be successfully preempted and thus release the main goroutine, but when it reached the end of this test case, the runtime.GC() cannot finish without panic, due to the way riscv64 port treats sp offset, which is not available for NOFRAME functions such as runtime.asyncPreempt. Some work has to be done here. On the other hand, in preprocess(), arm64 and ppc64 do recognize NOFRAME functions with zero stack size (textstksiz), but they don't purely rely on that to calculate Spadj.

The other issue is choosing which register to be clobbered. arm64 and MIPS choose REGTMP, and mark non-atomic call sequence to be not safe for async-preemption. I don't think choosing REGTMP (t6 in riscv64) is as good as previous supports, because there are many other cases that non-atomically use t6. I plan to just use REG_LR (ra in riscv64) here. ra is critical only when an RET instruction or the prologue in a function is async-preempted. For now I am not sure if it will be easier/cleaner than using REGTMP.

I expect to send this patch in a few weeks, hopefully not months.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Mar 28, 2020

I'm not sure LR is a good choice. For frameless leaf functions LR is live throughout the entire function. We don't want the whole function be non-preemptible.

On the other hand, the TMP register is only live across sequences of a small number of instructions. It is used for synthesize large offsets, etc. In particular, it won't make an entire loop non-preemptible. It looks to me the usage of REGTMP on RISCV is pretty much the same as on MIPS or ARM64. (Note that we only care code generated by the Go compiler, not assembly code.)

@NonerKao
Copy link
Contributor

@NonerKao NonerKao commented Mar 29, 2020

OK, I will check MIPS (69dcdbd) and ARM64 (4736088) to see how they mark REGTMP sequences.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2020

Change https://golang.org/cl/226206 mentions this issue: runtime: add async preemption support on riscv64

@gopherbot gopherbot closed this in b89f4c6 Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.