Repository: cilium/ebpf
Pull request #81 (2020-03-17 17:32:49)

Use number of possible CPUs for PerfEventArray.MaxEntries
Based on discussion in #58 and prompted by @iAklis and his PRs #78 and #80.

This fixes the behaviour on systems that have one or more CPUs disabled. Previously things would only work if the disabled CPUs were on the end of the range.

It's still not possible to use PerfEventArrays that have more than the total number of CPUs in the system (as suggested by @iAklis), since I'm not sure that's a good idea to do. We also don't deal with CPUs being added or removed after a perf.Reader has been created.

More details in the commit descriptions.


Patch:
diff --git a/internal/cpu.go b/internal/cpu.go
index b91550a..d3424ba 100644
--- a/internal/cpu.go
+++ b/internal/cpu.go
@@ -2,10 +2,9 @@ package internal
 
 import (
 	"fmt"
-	"os"
+	"io/ioutil"
+	"strings"
 	"sync"
-
-	"golang.org/x/xerrors"
 )
 
 var sysCPU struct {
@@ -18,45 +17,44 @@ var sysCPU struct {
 // Logical CPU numbers must be of the form 0-n
 func PossibleCPUs() (int, error) {
 	sysCPU.once.Do(func() {
-		sysCPU.num, sysCPU.err = parseCPUs("/sys/devices/system/cpu/possible")
+		sysCPU.num, sysCPU.err = parseCPUsFromFile("/sys/devices/system/cpu/possible")
 	})
 
 	return sysCPU.num, sysCPU.err
 }
 
-var onlineCPU struct {
-	once sync.Once
-	err  error
-	num  int
-}
+func parseCPUsFromFile(path string) (int, error) {
+	spec, err := ioutil.ReadFile(path)
+	if err != nil {
+		return 0, err
+	}
 
-// OnlineCPUs returns the number of currently online CPUs
-// Logical CPU numbers must be of the form 0-n
-func OnlineCPUs() (int, error) {
-	onlineCPU.once.Do(func() {
-		onlineCPU.num, onlineCPU.err = parseCPUs("/sys/devices/system/cpu/online")
-	})
+	n, err := parseCPUs(string(spec))
+	if err != nil {
+		return 0, fmt.Errorf("can't parse %s: %v", path, err)
+	}
 
-	return onlineCPU.num, onlineCPU.err
+	return n, nil
 }
 
-// parseCPUs parses the number of cpus from sysfs,
-// in the format of "/sys/devices/system/cpu/{possible,online,..}.
-// Logical CPU numbers must be of the form 0-n
-func parseCPUs(path string) (int, error) {
-	file, err := os.Open(path)
-	if err != nil {
-		return 0, err
+// parseCPUs parses the number of cpus from a string produced
+// by bitmap_list_string() in the Linux kernel.
+// Multiple ranges are rejected, since they can't be unified
+// into a single number.
+// This is the format of /sys/devices/system/cpu/possible, it
+// is not suitable for /sys/devices/system/cpu/online, etc.
+func parseCPUs(spec string) (int, error) {
+	if strings.Trim(spec, "\n") == "0" {
+		return 1, nil
 	}
-	defer file.Close()
 
 	var low, high int
-	n, _ := fmt.Fscanf(file, "%d-%d", &low, &high)
-	if n < 1 || low != 0 {
-		return 0, xerrors.Errorf("%s has unknown format: %v", path, err)
+	n, err := fmt.Sscanf(spec, "%d-%d\n", &low, &high)
+	if n != 2 || err != nil {
+		return 0, fmt.Errorf("invalid format: %s", spec)
 	}
-	if n == 1 {
-		high = low
+	if low != 0 {
+		return 0, fmt.Errorf("CPU spec doesn't start at zero: %s", spec)
 	}
 
 	// cpus is 0 indexed
diff --git a/internal/unix/types_linux.go b/internal/unix/types_linux.go
index 4e20320..7efbbd7 100644
--- a/internal/unix/types_linux.go
+++ b/internal/unix/types_linux.go
@@ -16,6 +16,7 @@ const (
 	EPOLLIN                  = linux.EPOLLIN
 	EINTR                    = linux.EINTR
 	ESRCH                    = linux.ESRCH
+	ENODEV                   = linux.ENODEV
 	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
diff --git a/internal/unix/types_other.go b/internal/unix/types_other.go
index 6af79ab..d85fdcf 100644
--- a/internal/unix/types_other.go
+++ b/internal/unix/types_other.go
@@ -17,6 +17,7 @@ const (
 	EINVAL                   = syscall.EINVAL
 	EINTR                    = syscall.EINTR
 	ESRCH                    = syscall.ESRCH
+	ENODEV                   = syscall.ENODEV
 	BPF_F_RDONLY_PROG        = 0
 	BPF_F_WRONLY_PROG        = 0
 	BPF_OBJ_NAME_LEN         = 0x10
diff --git a/map.go b/map.go
index dfad214..ddb8054 100644
--- a/map.go
+++ b/map.go
@@ -166,7 +166,7 @@ func createMap(spec *MapSpec, inner *internal.FD, handle *btf.Handle) (*Map, err
 		abi.ValueSize = 4
 
 		if abi.MaxEntries == 0 {
-			n, err := internal.OnlineCPUs()
+			n, err := internal.PossibleCPUs()
 			if err != nil {
 				return nil, xerrors.Errorf("perf event array: %w", err)
 			}
diff --git a/perf/reader.go b/perf/reader.go
index c8962ab..652c622 100644
--- a/perf/reader.go
+++ b/perf/reader.go
@@ -204,7 +204,9 @@ func NewReaderWithOptions(array *ebpf.Map, perCPUBuffer int, opts ReaderOptions)
 				unix.Close(fd)
 			}
 			for _, ring := range rings {
-				ring.Close()
+				if ring != nil {
+					ring.Close()
+				}
 			}
 		}
 	}()
@@ -214,8 +216,15 @@ func NewReaderWithOptions(array *ebpf.Map, perCPUBuffer int, opts ReaderOptions)
 	// Hence we have to create a ring for each CPU.
 	for i := 0; i < nCPU; i++ {
 		ring, err := newPerfEventRing(i, perCPUBuffer, opts.Watermark)
+		if xerrors.Is(err, unix.ENODEV) {
+			// The requested CPU is currently offline, skip it.
+			rings = append(rings, nil)
+			pauseFds = append(pauseFds, -1)
+			continue
+		}
+
 		if err != nil {
-			return nil, xerrors.Errorf("failed to create perf ring for CPU %d: %w", i, err)
+			return nil, xerrors.Errorf("failed to create perf ring for CPU %d: %v", i, err)
 		}
 		rings = append(rings, ring)
 		pauseFds = append(pauseFds, ring.fd)
@@ -253,9 +262,6 @@ func NewReaderWithOptions(array *ebpf.Map, perCPUBuffer int, opts ReaderOptions)
 	if err = pr.Resume(); err != nil {
 		return nil, err
 	}
-	if err = pr.Resume(); err != nil {
-		return nil, err
-	}
 	runtime.SetFinalizer(pr, (*Reader).Close)
 	return pr, nil
 }
@@ -293,7 +299,9 @@ func (pr *Reader) Close() error {
 
 		// Close rings
 		for _, ring := range pr.rings {
-			ring.Close()
+			if ring != nil {
+				ring.Close()
+			}
 		}
 		pr.rings = nil
 		pr.pauseFds = nil
@@ -378,7 +386,7 @@ func (pr *Reader) Pause() error {
 		return errClosed
 	}
 
-	for i := 0; i < len(pr.pauseFds); i++ {
+	for i := range pr.pauseFds {
 		if err := pr.array.Delete(uint32(i)); err != nil && !xerrors.Is(err, ebpf.ErrKeyNotExist) {
 			return xerrors.Errorf("could't delete event fd for CPU %d: %w", i, err)
 		}
@@ -398,9 +406,12 @@ func (pr *Reader) Resume() error {
 		return errClosed
 	}
 
-	for i := 0; i < len(pr.pauseFds); i++ {
-		fd := uint32(pr.pauseFds[i])
-		if err := pr.array.Put(uint32(i), fd); err != nil {
+	for i, fd := range pr.pauseFds {
+		if fd == -1 {
+			continue
+		}
+
+		if err := pr.array.Put(uint32(i), uint32(fd)); err != nil {
 			return xerrors.Errorf("couldn't put event fd %d for CPU %d: %w", fd, i, err)
 		}
 	}
diff --git a/perf/ring.go b/perf/ring.go
index f471285..4065429 100644
--- a/perf/ring.go
+++ b/perf/ring.go
@@ -34,7 +34,7 @@ func newPerfEventRing(cpu, perCPUBuffer, watermark int) (*perfEventRing, error)
 
 	fd, err := createPerfEvent(cpu, watermark)
 	if err != nil {
-		return nil, xerrors.Errorf("can't create perf event: %w", err)
+		return nil, err
 	}
 
 	if err := unix.SetNonblock(fd, true); err != nil {
@@ -90,7 +90,7 @@ func createPerfEvent(cpu, watermark int) (int, error) {
 	attr.Size = uint32(unsafe.Sizeof(attr))
 	fd, err := unix.PerfEventOpen(&attr, -1, cpu, -1, unix.PERF_FLAG_FD_CLOEXEC)
 	if err != nil {
-		return -1, xerrors.Errorf("can't create perf event: %v", err)
+		return -1, xerrors.Errorf("can't create perf event: %w", err)
 	}
 	return fd, nil
 }


Test patch:
diff --git a/internal/cpu_test.go b/internal/cpu_test.go
index 8a161d2..7ecee74 100644
--- a/internal/cpu_test.go
+++ b/internal/cpu_test.go
@@ -1,31 +1,32 @@
 package internal
 
 import (
-	"io"
-	"io/ioutil"
 	"testing"
 )
 
 func TestParseCPUs(t *testing.T) {
 	for str, result := range map[string]int{
-		"0-1": 2,
-		"0":   1,
+		"0-1":   2,
+		"0-2\n": 3,
+		"0":     1,
 	} {
-		fh, err := ioutil.TempFile("", "ebpf")
+		n, err := parseCPUs(str)
 		if err != nil {
-			t.Fatal(err)
-		}
-
-		if _, err := io.WriteString(fh, str); err != nil {
-			t.Fatal(err)
-		}
-		fh.Close()
-
-		n, err := parseCPUs(fh.Name())
-		if err != nil {
-			t.Error("Can't parse", str, err)
+			t.Errorf("Can't parse `%s`: %v", str, err)
 		} else if n != result {
 			t.Error("Parsing", str, "returns", n, "instead of", result)
 		}
 	}
+
+	for _, str := range []string{
+		"0,3-4",
+		"0-",
+		"1,",
+		"",
+	} {
+		_, err := parseCPUs(str)
+		if err == nil {
+			t.Error("Parsed invalid format:", str)
+		}
+	}
 }