Repository: cilium/ebpf
Pull request #70 (2020-02-27 16:50:48)

program: retry BPF_PROG_TEST_RUN on EINTR
Testing a BPF program can be interrupted if a signal arrives for a
userspace thread while being stuck in the BPF syscall.
BPF_PROG_TEST_RUN returns EINTR in that case, and we are required
to retry.

This has an unfortunate side-effect: benchmark results will be
skewed, because we might have to do more work than was originally
asked of us. The per-run calculation performed by the Go test
harness are therefore unreliable. Document this until we get a
better idea how to fix this.

Updates #24

lmb: > Another option would be to add an additional method, e.g. called BenchmarkWithB which takes a testing.(*B) on which it could then call ResetTimer itself on EINTR and retry.

That's kind of what I was going for in the beginning: `Benchmark(in []byte, repeat int, reset func())` (passing in testing.B is problematic, because pulling in testing creates command line flags). But then that changes the API, and I don't really know how much of a problem this is. So I figured I'd just fix the issue, and see if somebody complains.
tklauser: > > Another option would be to add an additional method, e.g. called BenchmarkWithB which takes a testing.(*B) on which it could then call ResetTimer itself on EINTR and retry.
> 
> That's kind of what I was going for in the beginning: `Benchmark(in []byte, repeat int, reset func())` (passing in testing.B is problematic, because pulling in testing creates command line flags). But then that changes the API, and I don't really know how much of a problem this is. So I figured I'd just fix the issue, and see if somebody complains.

Right, we don't want to pass in `testing.B`.

Maybe just add a second `BenchmarkWithReset` function that takes a `reset func()` and mark `Benchmark` as deprecated suggesting to use `BenchmarkWithReset`?
lmb: > Maybe just add a second BenchmarkWithReset function that takes a reset func() and mark Benchmark as deprecated suggesting to use BenchmarkWithReset?

Then we should just bite the bullet, and break the API. We've not made a stable release after all.
tklauser: sgtm
lmb: Updated the PR

Patch:
diff --git a/internal/unix/types_linux.go b/internal/unix/types_linux.go
index 0f27616..4e20320 100644
--- a/internal/unix/types_linux.go
+++ b/internal/unix/types_linux.go
@@ -14,6 +14,8 @@ const (
 	ENOSPC                   = linux.ENOSPC
 	EINVAL                   = linux.EINVAL
 	EPOLLIN                  = linux.EPOLLIN
+	EINTR                    = linux.EINTR
+	ESRCH                    = linux.ESRCH
 	BPF_F_RDONLY_PROG        = linux.BPF_F_RDONLY_PROG
 	BPF_F_WRONLY_PROG        = linux.BPF_F_WRONLY_PROG
 	BPF_OBJ_NAME_LEN         = linux.BPF_OBJ_NAME_LEN
@@ -127,3 +129,18 @@ type Utsname = linux.Utsname
 func Uname(buf *Utsname) (err error) {
 	return linux.Uname(buf)
 }
+
+// Getpid is a wrapper
+func Getpid() int {
+	return linux.Getpid()
+}
+
+// Gettid is a wrapper
+func Gettid() int {
+	return linux.Gettid()
+}
+
+// Tgkill is a wrapper
+func Tgkill(tgid int, tid int, sig syscall.Signal) (err error) {
+	return linux.Tgkill(tgid, tid, sig)
+}
diff --git a/internal/unix/types_other.go b/internal/unix/types_other.go
index 31353d4..6af79ab 100644
--- a/internal/unix/types_other.go
+++ b/internal/unix/types_other.go
@@ -15,6 +15,8 @@ const (
 	EAGAIN                   = syscall.EAGAIN
 	ENOSPC                   = syscall.ENOSPC
 	EINVAL                   = syscall.EINVAL
+	EINTR                    = syscall.EINTR
+	ESRCH                    = syscall.ESRCH
 	BPF_F_RDONLY_PROG        = 0
 	BPF_F_WRONLY_PROG        = 0
 	BPF_OBJ_NAME_LEN         = 0x10
@@ -193,3 +195,18 @@ type Utsname struct {
 func Uname(buf *Utsname) (err error) {
 	return errNonLinux
 }
+
+// Getpid is a wrapper
+func Getpid() int {
+	return -1
+}
+
+// Gettid is a wrapper
+func Gettid() int {
+	return -1
+}
+
+// Tgkill is a wrapper
+func Tgkill(tgid int, tid int, sig syscall.Signal) (err error) {
+	return errNonLinux
+}
diff --git a/prog.go b/prog.go
index a257d13..5306475 100644
--- a/prog.go
+++ b/prog.go
@@ -302,7 +302,7 @@ func (p *Program) Close() error {
 //
 // This function requires at least Linux 4.12.
 func (p *Program) Test(in []byte) (uint32, []byte, error) {
-	ret, out, _, err := p.testRun(in, 1)
+	ret, out, _, err := p.testRun(in, 1, nil)
 	if err != nil {
 		return ret, nil, xerrors.Errorf("can't test program: %w", err)
 	}
@@ -312,12 +312,16 @@ func (p *Program) Test(in []byte) (uint32, []byte, error) {
 // Benchmark runs the Program with the given input for a number of times
 // and returns the time taken per iteration.
 //
-// The returned value is the return value of the last execution of
-// the program.
+// Returns the result of the last execution of the program and the time per
+// run or an error. reset is called whenever the benchmark syscall is
+// interrupted, and should be set to testing.B.ResetTimer or similar.
+//
+// Note: profiling a call to this function will skew it's results, see
+// https://github.com/cilium/ebpf/issues/24
 //
 // This function requires at least Linux 4.12.
-func (p *Program) Benchmark(in []byte, repeat int) (uint32, time.Duration, error) {
-	ret, _, total, err := p.testRun(in, repeat)
+func (p *Program) Benchmark(in []byte, repeat int, reset func()) (uint32, time.Duration, error) {
+	ret, _, total, err := p.testRun(in, repeat, reset)
 	if err != nil {
 		return ret, total, xerrors.Errorf("can't benchmark program: %w", err)
 	}
@@ -359,7 +363,7 @@ var haveProgTestRun = internal.FeatureTest("BPF_PROG_TEST_RUN", "4.12", func() b
 	return !xerrors.Is(err, unix.EINVAL)
 })
 
-func (p *Program) testRun(in []byte, repeat int) (uint32, []byte, time.Duration, error) {
+func (p *Program) testRun(in []byte, repeat int, reset func()) (uint32, []byte, time.Duration, error) {
 	if uint(repeat) > math.MaxUint32 {
 		return 0, nil, 0, fmt.Errorf("repeat is too high")
 	}
@@ -397,8 +401,19 @@ func (p *Program) testRun(in []byte, repeat int) (uint32, []byte, time.Duration,
 		repeat:      uint32(repeat),
 	}
 
-	_, err = internal.BPF(_ProgTestRun, unsafe.Pointer(&attr), unsafe.Sizeof(attr))
-	if err != nil {
+	for {
+		_, err = internal.BPF(_ProgTestRun, unsafe.Pointer(&attr), unsafe.Sizeof(attr))
+		if err == nil {
+			break
+		}
+
+		if xerrors.Is(err, unix.EINTR) {
+			if reset != nil {
+				reset()
+			}
+			continue
+		}
+
 		return 0, nil, 0, xerrors.Errorf("can't run test: %w", err)
 	}
 


Test patch:
diff --git a/prog_test.go b/prog_test.go
index 369074f..9c1e334 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -7,12 +7,16 @@ import (
 	"math"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
+	"syscall"
 	"testing"
+	"time"
 
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
+	"golang.org/x/sys/unix"
 	"golang.org/x/xerrors"
 )
 
@@ -84,7 +88,7 @@ func TestProgramBenchmark(t *testing.T) {
 	prog := createSocketFilter(t)
 	defer prog.Close()
 
-	ret, duration, err := prog.Benchmark(make([]byte, 14), 1)
+	ret, duration, err := prog.Benchmark(make([]byte, 14), 1, nil)
 	testutils.SkipIfNotSupported(t, err)
 	if err != nil {
 		t.Fatal("Error from Benchmark:", err)
@@ -99,6 +103,74 @@ func TestProgramBenchmark(t *testing.T) {
 	}
 }
 
+func TestProgramTestRunInterrupt(t *testing.T) {
+	testutils.SkipOnOldKernel(t, "5.0", "EINTR from BPF_PROG_TEST_RUN")
+
+	prog := createSocketFilter(t)
+	defer prog.Close()
+
+	var (
+		tgid    = unix.Getpid()
+		tidChan = make(chan int, 1)
+		exit    = make(chan struct{})
+		errs    = make(chan error, 1)
+		timeout = time.After(5 * time.Second)
+	)
+
+	defer close(exit)
+
+	go func() {
+		runtime.LockOSThread()
+		defer func() {
+			// Wait for the test to allow us to unlock the OS thread, to
+			// ensure that we don't send SIGUSR1 to the wrong thread.
+			<-exit
+			runtime.UnlockOSThread()
+		}()
+
+		tidChan <- unix.Gettid()
+
+		// Block this thread in the BPF syscall, so that we can
+		// trigger EINTR by sending a signal.
+		_, _, _, err := prog.testRun(make([]byte, 14), math.MaxInt32, func() {
+			// We don't know how long finishing the
+			// test run would take, so flag that we've seen
+			// an interruption and abort the goroutine.
+			close(errs)
+			runtime.Goexit()
+		})
+
+		errs <- err
+	}()
+
+	tid := <-tidChan
+	for {
+		err := unix.Tgkill(tgid, tid, syscall.SIGUSR1)
+		if err != nil {
+			t.Fatal("Can't send signal to goroutine thread:", err)
+		}
+
+		select {
+		case err, ok := <-errs:
+			if !ok {
+				return
+			}
+
+			testutils.SkipIfNotSupported(t, err)
+			if err == nil {
+				t.Fatal("testRun wasn't interrupted")
+			}
+
+			t.Fatal("testRun returned an error:", err)
+
+		case <-timeout:
+			t.Fatal("Timed out trying to interrupt the goroutine")
+
+		default:
+		}
+	}
+}
+
 func TestProgramClose(t *testing.T) {
 	prog := createSocketFilter(t)