Skip to content

Commit

Permalink
map: use possible CPUs to size PerfEventArray
Browse files Browse the repository at this point in the history
We currently use the highest possible CPU to determine the
size of a PerfEventArray. This has several problems: first,
the code to parse the online CPUs doesn't correctly handle
ranges: "0,2-7" is treated like there is only one online CPU.
This can happen when a CPU is disabled at runtime using

    echo 0 | sudo tee /sys/devices/system/cpu/cpu1/online

We silently create a map with an incorrect size, and user
code starts to fail with E2BIG.

Fix this by using the number of possible CPUs as the map size.
The number can only be changed by a reboot and so is safe to use.
It's also simpler to use, since we don't have to deal with
multiple ranges like in the online CPU case.
  • Loading branch information
lmb authored and tklauser committed Mar 17, 2020
1 parent d313fc9 commit cbb0c67
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 46 deletions.
56 changes: 27 additions & 29 deletions internal/cpu.go
Expand Up @@ -2,10 +2,9 @@ package internal

import (
"fmt"
"os"
"io/ioutil"
"strings"
"sync"

"golang.org/x/xerrors"
)

var sysCPU struct {
Expand All @@ -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
Expand Down
33 changes: 17 additions & 16 deletions 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)
}
}
}
2 changes: 1 addition & 1 deletion map.go
Expand Up @@ -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)
}
Expand Down

0 comments on commit cbb0c67

Please sign in to comment.