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: check use of osyield in signal handlers #52672

Open
rhysh opened this issue May 3, 2022 · 5 comments
Open

runtime: check use of osyield in signal handlers #52672

rhysh opened this issue May 3, 2022 · 5 comments
Assignees
@mknyszek
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@rhysh
Copy link
Contributor

rhysh commented May 3, 2022

The SIGPROF handler uses runtime.osyield to spin while waiting for exclusive access to the profiling buffer. In the discussion of https://go.dev/cl/400795, @mknyszek questioned whether that was safe. I'm not sure that it is.

I see that on many OSes (Linux, FreeBSD, NetBSD, OpenBSD, Solaris, Dragonfly) that function does a sched_yield(2) syscall, which is not one of the syscalls listed in signal-safety(7) as being required by POSIX to be safe to call from signal handlers.

On top of that, the change in Go 1.18 to use timer_create(2) on Linux for CPU profiling means that many SIGPROF deliveries are now thread-targeted (versus setitimer(2)'s process-targeted signals), which may have opened the possibility of a process handling multiple overlapping SIGPROF deliveries (on separate threads).

Let's make sure the runtime.osyield calls are safe in that context, and write down why.

Here's some of the code in question: https://github.com/golang/go/blob/go1.18.1/src/runtime/cpuprof.go#L123-L129
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 3, 2022
@ianlancetaylor
Copy link
Contributor

Pure system calls are basically always async-signal-safe.

POSIX doesn't require sched_yield to be async-signal-safe because POSIX doesn't require threads to be implemented in the kernel. But for those cases where osyield is just a system call, we are fine.

@mknyszek
Copy link
Contributor

mknyszek commented May 3, 2022

Thanks Ian. A bunch of platforms do call sched_yield through libc (or some other shared library). Presumably that's done just because we do everything through libc on those platforms, and really it's just a simple syscall under the hood. I suppose we just have to trust that and document it.

@mknyszek mknyszek self-assigned this May 3, 2022
@mknyszek mknyszek modified the milestones: Gccgo, Backlog May 3, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@mknyszek mknyszek modified the milestones: Backlog, Go1.21 Jun 16, 2023
@mknyszek
Copy link
Contributor

Pushing this to Go 1.22.

@mknyszek mknyszek modified the milestones: Go1.21, Go1.22 Jun 30, 2023
@bcmills
Copy link
Contributor

bcmills commented Aug 9, 2023

Presumably that's done just because we do everything through libc on those platforms, and really it's just a simple syscall under the hood.

How sure are we about that, especially in the context of the race detector?
I wonder if this may be responsible for runtime/pprof deadlocks on darwin-amd64-race:

@mknyszek
Copy link
Contributor

mknyszek commented Aug 9, 2023

I'm certainly less sure than I used to be!

@mknyszek mknyszek modified the milestones: Go1.22, Backlog Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mknyszek mknyszek

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rhysh @mknyszek @dr2chase @ianlancetaylor @bcmills @gopherbot