Repository: cilium/ebpf
Pull request #88 (2020-04-21 09:59:14)

perf: Fix mmap buffer size calculation
The mmap buffer is sized in bytes, but needs to be page aligned.
This was correctly derived.

Additionally, with perf_event_open, according to man 2 perf_event_open:

> The mmap size should be 1+2^n pages

This wasn't correctly enforced, and caused the mmap call to fail with
EINVAL for some buffer sizes. For example 8193 bytes would be
incorrectly rounded to 3 pages instead of 4.
Always round up to the nearest power of two number of pages, and add
some tests.

Also allow `watermark == buffer` as this as a source of confusion, 
see https://github.com/cloudflare/xdpcap/issues/39#issuecomment-616382309


arthurfabre: > Thanks for taking this on :) While reading your test I had the following idea: what if we just reject buffer sizes that aren't a power of two already? That's a change in API, but we can probably audit the users of the perf reader for this. Downside: users have another hoop to jump through, upside: watermark and buffer size are now consistent.

Thanks for having a look :) Buffer sizes have to be a power of two number of pages, I don't get how requiring power of two buffer sizes helps? Or would users just specify a number of pages, and we would multiply it by the page size?
lmb: > Buffer sizes have to be a power of two number of pages, I don't get how requiring power of two buffer sizes helps? Or would users just specify a number of pages, and we would multiply it by the page size?

Ok, no that was a terrible idea of mine. Disregard!
lmb: Can you fix the wrapping and rebase on master please?
arthurfabre: Forced pushed again to re-run the builds, AFAICT https://ebpf.semaphoreci.com/jobs/5af77b40-756d-47b2-88a2-c1a11dbab61a seems to have just timed out. I diffed the output with a successful build on the same kernel (https://ebpf.semaphoreci.com/jobs/c59893bb-e772-4727-b8f7-899c2eb0de2c), and it doesn't seem like any tests were blocked or deadlocked, every `== RUN` had a matching `--- PASS`.

Patch:
diff --git a/perf/ring.go b/perf/ring.go
index 4065429..a353a5f 100644
--- a/perf/ring.go
+++ b/perf/ring.go
@@ -2,6 +2,7 @@ package perf
 
 import (
 	"io"
+	"math"
 	"os"
 	"runtime"
 	"sync/atomic"
@@ -26,12 +27,6 @@ func newPerfEventRing(cpu, perCPUBuffer, watermark int) (*perfEventRing, error)
 		return nil, xerrors.New("watermark must be smaller than perCPUBuffer")
 	}
 
-	// Round to nearest page boundary and allocate
-	// an extra page for meta data
-	pageSize := os.Getpagesize()
-	nPages := (perCPUBuffer + pageSize - 1) / pageSize
-	size := (1 + nPages) * pageSize
-
 	fd, err := createPerfEvent(cpu, watermark)
 	if err != nil {
 		return nil, err
@@ -42,10 +37,10 @@ func newPerfEventRing(cpu, perCPUBuffer, watermark int) (*perfEventRing, error)
 		return nil, err
 	}
 
-	mmap, err := unix.Mmap(fd, 0, size, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED)
+	mmap, err := unix.Mmap(fd, 0, perfBufferSize(perCPUBuffer), unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED)
 	if err != nil {
 		unix.Close(fd)
-		return nil, err
+		return nil, xerrors.Errorf("can't mmap: %v", err)
 	}
 
 	// This relies on the fact that we allocate an extra metadata page,
@@ -65,6 +60,22 @@ func newPerfEventRing(cpu, perCPUBuffer, watermark int) (*perfEventRing, error)
 	return ring, nil
 }
 
+// mmapBufferSize returns a valid mmap buffer size for use with perf_event_open (1+2^n pages)
+func perfBufferSize(perCPUBuffer int) int {
+	pageSize := os.Getpagesize()
+
+	// Smallest whole number of pages
+	nPages := (perCPUBuffer + pageSize - 1) / pageSize
+
+	// Round up to nearest power of two number of pages
+	nPages = int(math.Pow(2, math.Ceil(math.Log2(float64(nPages)))))
+
+	// Add one for metadata
+	nPages += 1
+
+	return nPages * pageSize
+}
+
 func (ring *perfEventRing) Close() {
 	runtime.SetFinalizer(ring, nil)
 	unix.Close(ring.fd)


Test patch:
diff --git a/perf/ring_test.go b/perf/ring_test.go
index 60796cb..1b7d7d1 100644
--- a/perf/ring_test.go
+++ b/perf/ring_test.go
@@ -3,6 +3,7 @@ package perf
 import (
 	"bytes"
 	"io"
+	"os"
 	"testing"
 
 	"github.com/cilium/ebpf/internal/unix"
@@ -62,3 +63,46 @@ func makeRing(size, offset int) *ringReader {
 
 	return newRingReader(&meta, ring)
 }
+
+func TestPerfEventRing(t *testing.T) {
+	check := func(buffer, watermark int) {
+		ring, err := newPerfEventRing(0, buffer, watermark)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		size := len(ring.ringReader.ring)
+
+		// Ring size should be at least as big as buffer
+		if size < buffer {
+			t.Fatalf("ring size %d smaller than buffer %d", size, buffer)
+		}
+
+		// Ring size should be of the form 2^n pages (meta page has already been removed)
+		if size%os.Getpagesize() != 0 {
+			t.Fatalf("ring size %d not whole number of pages (pageSize %d)", size, os.Getpagesize())
+		}
+		nPages := size / os.Getpagesize()
+		if nPages&(nPages-1) != 0 {
+			t.Fatalf("ring size %d (%d pages) not a power of two pages (pageSize %d)", size, nPages, os.Getpagesize())
+		}
+	}
+
+	// watermark > buffer
+	_, err := newPerfEventRing(0, 8192, 8193)
+	if err == nil {
+		t.Fatal("watermark > buffer allowed")
+	}
+
+	// watermark == buffer
+	_, err = newPerfEventRing(0, 8192, 8192)
+	if err == nil {
+		t.Fatal("watermark == buffer allowed")
+	}
+
+	// buffer not a power of two, watermark < buffer
+	check(8193, 8192)
+
+	// large buffer not a multiple of page size at all (prime)
+	check(65537, 8192)
+}