Repository: cilium/ebpf
Pull request #887 (2022-12-12 11:40:03)

program: block SIGPROF during BPF_PROG_RUN
Extend the profiler blocking we introduced for BPF_PROG_LOAD to BPF_PROG_RUN, with the difference that EINTR is not handled in the syscall wrapper itself. This allows us to have PROG_RUN specific retry logic.

Deprecate RunOptions.Reset since its main motivation was to account for profiler skew. Interruptions from the runtime are only possible via runtime.AllThreadSyscall, which is very specialised anyways. This doesn't protect from interruptions due to signals that are being sent from "outside" the process, which will still skew results and cause retries.

Document that RunOptions.Repeat is a lower bound for how often a program is run due to the retry behaviour. Also add a workaround that makes the common case of repeat == 1 not suffer from interruptions.

Fixes https://github.com/cilium/ebpf/issues/853


Patch:
diff --git a/internal/sys/syscall.go b/internal/sys/syscall.go
index 3ffcd7a..3c7ce5d 100644
--- a/internal/sys/syscall.go
+++ b/internal/sys/syscall.go
@@ -19,7 +19,7 @@ var ENOTSUPP = syscall.Errno(524)
 func BPF(cmd Cmd, attr unsafe.Pointer, size uintptr) (uintptr, error) {
 	// Prevent the Go profiler from repeatedly interrupting the verifier,
 	// which could otherwise lead to a livelock due to receiving EAGAIN.
-	if cmd == BPF_PROG_LOAD {
+	if cmd == BPF_PROG_LOAD || cmd == BPF_PROG_RUN {
 		maskProfilerSignal()
 		defer unmaskProfilerSignal()
 	}
diff --git a/prog.go b/prog.go
index b5a9dce..a113a5f 100644
--- a/prog.go
+++ b/prog.go
@@ -503,6 +503,9 @@ func (p *Program) Close() error {
 // Various options for Run'ing a Program
 type RunOptions struct {
 	// Program's data input. Required field.
+	//
+	// The kernel expects at least 14 bytes input for an ethernet header for
+	// XDP and SKB programs.
 	Data []byte
 	// Program's data after Program has run. Caller must allocate. Optional field.
 	DataOut []byte
@@ -510,7 +513,10 @@ type RunOptions struct {
 	Context interface{}
 	// Program's context after Program has run. Must be a pointer or slice. Optional field.
 	ContextOut interface{}
-	// Number of times to run Program. Optional field. Defaults to 1.
+	// Minimum number of times to run Program. Optional field. Defaults to 1.
+	//
+	// The program may be executed more often than this due to interruptions, e.g.
+	// when runtime.AllThreadsSyscall is invoked.
 	Repeat uint32
 	// Optional flags.
 	Flags uint32
@@ -519,6 +525,8 @@ type RunOptions struct {
 	CPU uint32
 	// Called whenever the syscall is interrupted, and should be set to testing.B.ResetTimer
 	// or similar. Typically used during benchmarking. Optional field.
+	//
+	// Deprecated: use [testing.B.ReportMetric] with unit "ns/op" instead.
 	Reset func()
 }
 
@@ -546,9 +554,9 @@ func (p *Program) Test(in []byte) (uint32, []byte, error) {
 		Repeat:  1,
 	}
 
-	ret, _, err := p.testRun(&opts)
+	ret, _, err := p.run(&opts)
 	if err != nil {
-		return ret, nil, fmt.Errorf("can't test program: %w", err)
+		return ret, nil, fmt.Errorf("test program: %w", err)
 	}
 	return ret, opts.DataOut, nil
 }
@@ -557,9 +565,9 @@ func (p *Program) Test(in []byte) (uint32, []byte, error) {
 //
 // Note: the same restrictions from Test apply.
 func (p *Program) Run(opts *RunOptions) (uint32, error) {
-	ret, _, err := p.testRun(opts)
+	ret, _, err := p.run(opts)
 	if err != nil {
-		return ret, fmt.Errorf("can't test program: %w", err)
+		return ret, fmt.Errorf("run program: %w", err)
 	}
 	return ret, nil
 }
@@ -586,14 +594,14 @@ func (p *Program) Benchmark(in []byte, repeat int, reset func()) (uint32, time.D
 		Reset:  reset,
 	}
 
-	ret, total, err := p.testRun(&opts)
+	ret, total, err := p.run(&opts)
 	if err != nil {
-		return ret, total, fmt.Errorf("can't benchmark program: %w", err)
+		return ret, total, fmt.Errorf("benchmark program: %w", err)
 	}
 	return ret, total, nil
 }
 
-var haveProgTestRun = internal.NewFeatureTest("BPF_PROG_TEST_RUN", "4.12", func() error {
+var haveProgRun = internal.NewFeatureTest("BPF_PROG_RUN", "4.12", func() error {
 	prog, err := NewProgram(&ProgramSpec{
 		// SocketFilter does not require privileges on newer kernels.
 		Type: SocketFilter,
@@ -637,12 +645,12 @@ var haveProgTestRun = internal.NewFeatureTest("BPF_PROG_TEST_RUN", "4.12", func(
 	return err
 })
 
-func (p *Program) testRun(opts *RunOptions) (uint32, time.Duration, error) {
+func (p *Program) run(opts *RunOptions) (uint32, time.Duration, error) {
 	if uint(len(opts.Data)) > math.MaxUint32 {
 		return 0, 0, fmt.Errorf("input is too long")
 	}
 
-	if err := haveProgTestRun(); err != nil {
+	if err := haveProgRun(); err != nil {
 		return 0, 0, err
 	}
 
@@ -675,24 +683,45 @@ func (p *Program) testRun(opts *RunOptions) (uint32, time.Duration, error) {
 		Cpu:         opts.CPU,
 	}
 
+	if attr.Repeat == 0 {
+		attr.Repeat = 1
+	}
+
+retry:
 	for {
 		err := sys.ProgRun(&attr)
 		if err == nil {
-			break
+			break retry
 		}
 
 		if errors.Is(err, unix.EINTR) {
+			if attr.Repeat == 1 {
+				// Older kernels check whether enough repetitions have been
+				// executed only after checking for pending signals.
+				//
+				//     run signal? done? run ...
+				//
+				// As a result we can get EINTR for repeat==1 even though
+				// the program was run exactly once. Treat this as a
+				// successful run instead.
+				//
+				// Since commit 607b9cc92bd7 ("bpf: Consolidate shared test timing code")
+				// the conditions are reversed:
+				//     run done? signal? ...
+				break retry
+			}
+
 			if opts.Reset != nil {
 				opts.Reset()
 			}
-			continue
+			continue retry
 		}
 
 		if errors.Is(err, sys.ENOTSUPP) {
-			return 0, 0, fmt.Errorf("kernel doesn't support testing program type %s: %w", p.Type(), ErrNotSupported)
+			return 0, 0, fmt.Errorf("kernel doesn't support running %s: %w", p.Type(), ErrNotSupported)
 		}
 
-		return 0, 0, fmt.Errorf("can't run test: %w", err)
+		return 0, 0, err
 	}
 
 	if opts.DataOut != nil {


Test patch:
diff --git a/prog_test.go b/prog_test.go
index 3d615a9..583349f 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -230,7 +230,7 @@ func TestProgramTestRunInterrupt(t *testing.T) {
 				runtime.Goexit()
 			},
 		}
-		_, _, err := prog.testRun(&opts)
+		_, _, err := prog.run(&opts)
 
 		errs <- err
 	}()
@@ -636,7 +636,7 @@ func TestProgramFromFD(t *testing.T) {
 }
 
 func TestHaveProgTestRun(t *testing.T) {
-	testutils.CheckFeatureTest(t, haveProgTestRun)
+	testutils.CheckFeatureTest(t, haveProgRun)
 }
 
 func TestProgramGetNextID(t *testing.T) {