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: async signals not reliably delivered to Go threads under TSAN #18717

Closed
bcmills opened this issue Jan 19, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@bcmills
Copy link
Member

commented Jan 19, 2017

go version devel +f4be767501 Wed Jan 18 16:21:28 2017 -0500 linux/amd64

The fix for #17753 causes signal delivery in Go programs running under TSAN to pass through the TSAN interceptors.

Unfortunately, it appears that the TSAN interceptors defer delivery of asynchronous signals until the next blocking libc call on the same thread. This interacts poorly with the Go runtime's use of direct 'futex' syscalls to park idle threads: if the thread receiving the signal happens to be one that the runtime is about to park, it may never make a libc call and the signal will be lost.

A simple program illustrating this behavior:

src/tsansig/tsansig.go:

package main

import (
	"os"
	"os/signal"
	"syscall"
)

/*
#cgo CFLAGS: -g -fsanitize=thread
#cgo LDFLAGS: -g -fsanitize=thread
*/
import "C"

func main() {
	c := make(chan os.Signal, 1)
	signal.Notify(c, syscall.SIGUSR1)
	defer signal.Stop(c)
	syscall.Kill(syscall.Getpid(), syscall.SIGUSR1)
	<-c
}
$ go build tsansig
$ ./tsansig

It should exit with status 0 almost immediately. Instead, it hangs forever.

One might expect that injecting a blocking libc call would help:

package main

import (
	"os"
	"os/signal"
	"syscall"
	"time"
)

/*
#cgo CFLAGS: -g -fsanitize=thread
#cgo LDFLAGS: -g -fsanitize=thread

#include <stddef.h>
#include <time.h>
*/
import "C"

func nanosleep(d time.Duration) error {
	req := C.struct_timespec{
		tv_sec:  C.__time_t(d / time.Second),
		tv_nsec: C.__syscall_slong_t(d % time.Second),
	}
	var rem C.struct_timespec
	for {
		switch _, err := C.nanosleep(&req, &rem); err {
		case nil:
			return nil
		case syscall.EINTR:
			req = rem
			continue
		default:
			return err
		}
	}
}

func main() {
	c := make(chan os.Signal, 1)
	signal.Notify(c, syscall.SIGUSR1)
	defer signal.Stop(c)
	syscall.Kill(syscall.Getpid(), syscall.SIGUSR1)

	for {
		select {
		case <-c:
			return
		default:
			if err := nanosleep(10 * time.Nanosecond); err != nil {
				panic(err)
			}
		}
	}
}

But that attempt at a workaround does not succeed: the thread that receives the signal happens not to be the one the runtime uses for the subsequent cgo calls.

Impact on Go 1.8

This is (unfortunately) a significant regression over Go 1.7. In 1.7, Go programs without significant C signal behavior would exhibit correct Go signal behavior. In the current 1.8 RC, the C signal behavior in the presence of the Go runtime is improved, but the Go signal behavior is broken even for programs with no interesting C signal activity.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

One possible fix would be to have something in the runtime make an explicit call (in cgo-enabled builds) to some no-op libc function (e.g. sleep(0)) to trigger signal delivery. (runtime.findrunnable seems like a likely candidate.)

However, it isn't obvious to me that any call to runtime.futexsleep with a negative ns parameter can be made safely in the presence of TSAN, since the signal could be delivered at any point between the last libc call and the futex syscall. Since futexsleep allows spurious wakeups, perhaps we also need to inject an arbitrary timeout to futexsleep when TSAN is enabled to bound the delay on signal delivery.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@dvyukov

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

Tsan has a notion of "blocking regions" of code (e.g. pthread_cond_wait). It delivers signals synchronously in such regions as it knows that no activity happens on the thread. We could mark whole Go world as "blocking region", then tsan will deliver signals synchronously while in Go code. However, we call some tsan annotations (__tsan_acquire/release) from Go code, so we will need to somehow mark them as well.
This will require new hooks in tsan runtime, so we will need to handle versioning somehow as old tsan runtime won't have the hooks.
What release is this for?

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

We could mark whole Go world as "blocking region", then tsan will deliver signals synchronously while in Go code.

This will require new hooks in tsan runtime

That seems much more complicated than making periodic libc calls on idle Ms — we would also need to unblock the regions during cgo calls.

What release is this for?

I was trying to fix it in 1.8, but the changes are invasive enough that Ian suggested we delay until 1.9.

@bcmills bcmills changed the title runtime: async signals not reliably delivered to Go threads under TSAN/MSAN runtime: async signals not reliably delivered to Go threads under TSAN Jan 20, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jan 24, 2017

CL https://golang.org/cl/35494 mentions this issue.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

I've been thinking more about @dvyukov's comments, and I'm starting to see the logic to it.

I agree that marking the whole Go runtime as a "blocking region" for TSAN purposes is the right first-principles solution. It seems that as far as TSAN is concerned, the important property of a "blocking call" is that it does not acquire any libc locks, and Go only acquires libc locks when executing cgo function calls (including the x_cgo hooks in place for other runtime calls).

That said, the "periodic yielding" strategy does seem to improve responsiveness significantly in practice, so I think that's probably the right fallback for versions of TSAN which do not support the proposed new hooks.

@gopherbot gopherbot closed this in 29edf0f Mar 8, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Mar 9, 2017

CL https://golang.org/cl/37867 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 9, 2017

runtime: fix _cgo_yield usage with sysmon and on BSD
There are a few problems from change 35494, discovered during testing
of change 37852.

1. I was confused about the usage of n.key in the sema variant, so we
   were looping on the wrong condition. The error was not caught by
   the TryBots (presumably due to missing TSAN coverage in the BSD and
   darwin builders?).

2. The sysmon goroutine sometimes skips notetsleep entirely, using
   direct usleep syscalls instead. In that case, we were not calling
   _cgo_yield, leading to missed signals under TSAN.

3. Some notetsleep calls have long finite timeouts. They should be
   broken up into smaller chunks with a yield at the end of each
   chunk.

updates #18717

Change-Id: I91175af5dea3857deebc686f51a8a40f9d690bcc
Reviewed-on: https://go-review.googlesource.com/37867
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang locked and limited conversation to collaborators Mar 9, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.