perf(ebpf): cache uprobe symbol lookups by binary identity

ebpftracer/nodejs.go

@@ -2,6 +2,7 @@ package ebpftracer
 
 import (
 	"bufio"
+	"fmt"
 	"os"
 	"strings"
 
@@ -11,6 +12,16 @@ import (
 	"k8s.io/klog/v2"
 )
 
+const nodejsPollSymbol = "uv__io_poll"
+
+// libuv I/O callbacks. Not every libuv build exports all of them; the attach
+// loop stops at the first one missing, matching the previous behaviour.
+var nodejsCallbackSymbols = []string{"uv__stream_io", "uv__async_io", "uv__poll_io", "uv__server_io", "uv__udp_io"}
+
+// Every symbol resolved in one pass over the ELF symbol table, so a cache miss
+// costs a single parse rather than one per symbol.
+var nodejsProbeSymbols = append([]string{nodejsPollSymbol}, nodejsCallbackSymbols...)
+
 func (t *Tracer) AttachNodejsProbes(pid uint32, exe string) []link.Link {
 	log := func(libPath, msg string, err error) {
 		if err != nil {
@@ -44,24 +55,25 @@ func (t *Tracer) attachNodejsUprobes(libPath string, pid uint32) ([]link.Link, e
 	if err != nil {
 		return nil, err
 	}
-	ef, err := OpenELFFile(libPath)
+
+	// Resolved once per binary rather than once per pid — see LookupSymbols.
+	targets, err := LookupSymbols(libPath, nodejsProbeSymbols)
 	if err != nil {
 		return nil, err
 	}
-	defer ef.Close()
 
-	s, err := ef.GetSymbol("uv__io_poll")
-	if err != nil {
-		return nil, err
+	poll := targets[nodejsPollSymbol]
+	if !poll.Found {
+		return nil, fmt.Errorf("symbol %s not found", nodejsPollSymbol)
 	}
-	l, err := s.AttachUprobe(exe, t.uprobes["uv_io_poll_enter"], pid)
+	l, err := attachUprobeAt(exe, t.uprobes["uv_io_poll_enter"], pid, poll.Address)
 	if err != nil {
 		return nil, err
 	}
 	var links []link.Link
 	links = append(links, l)
 
-	ls, err := s.AttachUretprobes(exe, t.uprobes["uv_io_poll_exit"], pid)
+	ls, err := attachUretprobesAt(exe, t.uprobes["uv_io_poll_exit"], pid, poll)
 	links = append(links, ls...)
 	if err != nil {
 		for _, l := range links {
@@ -70,17 +82,17 @@ func (t *Tracer) attachNodejsUprobes(libPath string, pid uint32) ([]link.Link, e
 		return nil, err
 	}
 
-	for _, cb := range []string{"uv__stream_io", "uv__async_io", "uv__poll_io", "uv__server_io", "uv__udp_io"} {
-		s, err = ef.GetSymbol(cb)
-		if err != nil {
+	for _, cb := range nodejsCallbackSymbols {
+		target := targets[cb]
+		if !target.Found {
 			break
 		}
-		l, err = s.AttachUprobe(exe, t.uprobes["uv_io_cb_enter"], pid)
+		l, err = attachUprobeAt(exe, t.uprobes["uv_io_cb_enter"], pid, target.Address)
 		if err != nil {
 			break
 		}
 		links = append(links, l)
-		ls, err = s.AttachUretprobes(exe, t.uprobes["uv_io_cb_exit"], pid)
+		ls, err = attachUretprobesAt(exe, t.uprobes["uv_io_cb_exit"], pid, target)
 		links = append(links, ls...)
 		if err != nil {
 			break

ebpftracer/symbol_cache.go

@@ -0,0 +1,184 @@
+package ebpftracer
+
+import (
+	"fmt"
+	"os"
+	"sync"
+	"syscall"
+
+	"github.com/cilium/ebpf"
+	"github.com/cilium/ebpf/link"
+	lru "github.com/hashicorp/golang-lru/v2"
+)
+
+// ProbeTarget is everything uprobe attachment needs from a symbol: the file
+// offset to attach at, and the offsets of the RET instructions inside the
+// function for uretprobes.
+//
+// Both are pure functions of (binary, symbol name) — nothing about them is
+// per-process — so they can be resolved once and reused for every process
+// running that binary.
+type ProbeTarget struct {
+	Address       uint64
+	ReturnOffsets []int
+	Found         bool
+}
+
+// binaryKey identifies a file by identity rather than by path.
+//
+// Paths here are per-process (/proc/<pid>/root/usr/bin/node), so keying the
+// cache on the path string would miss for every new pid — exactly the case the
+// cache exists to eliminate. Pods from the same image share the read-only
+// overlay layer, so dev+inode collapses them onto one entry. Size and mtime
+// guard against inode reuse after a delete.
+type binaryKey struct {
+	dev, ino, size uint64
+	mtimeNsec      int64
+}
+
+// symbolKey caches one symbol at a time rather than one map per binary.
+//
+// Keying only by binary and storing the map the first caller happened to ask
+// for is wrong: a later caller wanting different symbols from the same file
+// would be handed that map and read every one of its own symbols as absent.
+// The Node.js and Go-TLS probes can both target the same executable, so this is
+// reachable, not theoretical.
+type symbolKey struct {
+	bin  binaryKey
+	name string
+}
+
+// Bounded so a node cycling through many image versions cannot grow this
+// without limit. Entries are a handful of ints each.
+const symbolCacheSize = 4096
+
+var (
+	symbolCacheMu sync.Mutex
+	symbolCache   *lru.Cache[symbolKey, ProbeTarget]
+)
+
+func init() {
+	symbolCache, _ = lru.New[symbolKey, ProbeTarget](symbolCacheSize)
+}
+
+func binaryKeyFor(path string) (binaryKey, error) {
+	fi, err := os.Stat(path)
+	if err != nil {
+		return binaryKey{}, err
+	}
+	st, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {
+		return binaryKey{}, fmt.Errorf("stat unavailable for %s", path)
+	}
+	return binaryKey{
+		dev:       uint64(st.Dev),
+		ino:       uint64(st.Ino),
+		size:      uint64(fi.Size()),
+		mtimeNsec: fi.ModTime().UnixNano(),
+	}, nil
+}
+
+// LookupSymbols resolves the named symbols in the binary at path.
+//
+// On a miss it reads the ELF symbol table and disassembles each function body
+// to find its RET instructions; on a hit it does neither. That matters because
+// the uncached path is expensive out of proportion to what it yields: node is a
+// large statically linked binary, and readSymbols loads the whole .symtab and
+// .dynsym — allocating a Go string per symbol — to locate six functions. A
+// customer profile showed this at 17.5% of a core, repeated per pid, because
+// instrumentNodejs is gated per Process and Node cluster mode runs many
+// processes off one binary.
+//
+// Symbols that are absent are cached with Found=false, deliberately: a binary
+// that does not export the probe points must not be re-parsed for every new
+// pid. That negative case is the common one, since every non-Node executable
+// that reaches here also fails to match.
+func LookupSymbols(path string, names []string) (map[string]ProbeTarget, error) {
+	key, err := binaryKeyFor(path)
+	if err != nil {
+		return nil, err
+	}
+
+	targets := make(map[string]ProbeTarget, len(names))
+	var missing []string
+	symbolCacheMu.Lock()
+	for _, name := range names {
+		if t, ok := symbolCache.Get(symbolKey{bin: key, name: name}); ok {
+			targets[name] = t
+		} else {
+			missing = append(missing, name)
+		}
+	}
+	symbolCacheMu.Unlock()
+
+	if len(missing) == 0 {
+		return targets, nil
+	}
+
+	// One parse resolves every name still missing, so a partial hit costs no
+	// more than a full miss.
+	resolved, err := readProbeTargets(path, missing)
+	if err != nil {
+		// Could not read the binary at all — not cacheable, since a later
+		// attempt against a readable path may succeed.
+		return nil, err
+	}
+
+	symbolCacheMu.Lock()
+	for name, t := range resolved {
+		symbolCache.Add(symbolKey{bin: key, name: name}, t)
+		targets[name] = t
+	}
+	symbolCacheMu.Unlock()
+	return targets, nil
+}
+
+// readProbeTargets does the expensive work: one ELF open, one symbol table
+// parse shared across all requested names, and one disassembly per found
+// symbol.
+func readProbeTargets(path string, names []string) (map[string]ProbeTarget, error) {
+	ef, err := OpenELFFile(path)
+	if err != nil {
+		return nil, err
+	}
+	defer ef.Close()
+
+	targets := make(map[string]ProbeTarget, len(names))
+	for _, name := range names {
+		s, err := ef.GetSymbol(name)
+		if err != nil {
+			targets[name] = ProbeTarget{}
+			continue
+		}
+		t := ProbeTarget{Address: s.Address(), Found: true}
+		// A symbol with no discoverable RET offsets still attaches an entry
+		// uprobe; only the uretprobes are skipped.
+		if offsets, err := s.ReturnOffsets(); err == nil {
+			t.ReturnOffsets = offsets
+		}
+		targets[name] = t
+	}
+	return targets, nil
+}
+
+// attachUprobeAt and attachUretprobesAt mirror Symbol.AttachUprobe and
+// Symbol.AttachUretprobes, but take a resolved address so attachment no longer
+// requires holding an open ELFFile.
+func attachUprobeAt(exe *link.Executable, prog *ebpf.Program, pid uint32, addr uint64) (link.Link, error) {
+	return exe.Uprobe("", prog, &link.UprobeOptions{Address: addr, PID: int(pid)})
+}
+
+func attachUretprobesAt(exe *link.Executable, prog *ebpf.Program, pid uint32, t ProbeTarget) ([]link.Link, error) {
+	if len(t.ReturnOffsets) == 0 {
+		return nil, fmt.Errorf("no return offsets")
+	}
+	var links []link.Link
+	for _, offset := range t.ReturnOffsets {
+		l, err := exe.Uprobe("", prog, &link.UprobeOptions{Address: t.Address + uint64(offset), PID: int(pid)})
+		if err != nil {
+			return links, err
+		}
+		links = append(links, l)
+	}
+	return links, nil
+}

ebpftracer/symbol_cache_test.go

@@ -0,0 +1,105 @@
+package ebpftracer
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+)
+
+// The cache must key on file identity, not on the path string. Probe paths are
+// per-process (/proc/<pid>/root/...) and every pid produces a different string
+// for the same file, so a path-keyed cache would miss every time — which is the
+// behaviour this cache exists to remove.
+func TestLookupSymbolsCachesByFileIdentity(t *testing.T) {
+	bin, err := exec.LookPath("true")
+	if err != nil {
+		t.Skip("no /bin/true available")
+	}
+
+	first, err := LookupSymbols(bin, []string{"main"})
+	if err != nil {
+		t.Fatalf("first lookup: %v", err)
+	}
+
+	// A second path referring to the same inode must hit the same entry.
+	link := filepath.Join(t.TempDir(), "true-hardlink")
+	if err := os.Link(bin, link); err != nil {
+		t.Skipf("cannot hardlink %s: %v", bin, err)
+	}
+	second, err := LookupSymbols(link, []string{"main"})
+	if err != nil {
+		t.Fatalf("lookup via hardlink: %v", err)
+	}
+
+	k1, err := binaryKeyFor(bin)
+	if err != nil {
+		t.Fatal(err)
+	}
+	k2, err := binaryKeyFor(link)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if k1 != k2 {
+		t.Fatalf("same file yielded different cache keys:\n  %+v\n  %+v", k1, k2)
+	}
+	if len(first) != len(second) {
+		t.Errorf("cached result differs: %d vs %d entries", len(first), len(second))
+	}
+}
+
+// A binary that does not export the probe points must be recorded as
+// Found=false rather than erroring, so it is never re-parsed. Non-Node
+// executables reaching this path are the common case, and re-parsing them per
+// pid is what made this hot.
+func TestLookupSymbolsCachesNegativeResults(t *testing.T) {
+	bin, err := exec.LookPath("true")
+	if err != nil {
+		t.Skip("no /bin/true available")
+	}
+	targets, err := LookupSymbols(bin, []string{"definitely_not_a_real_symbol_xyzzy"})
+	if err != nil {
+		t.Fatalf("absent symbol should not error: %v", err)
+	}
+	got, ok := targets["definitely_not_a_real_symbol_xyzzy"]
+	if !ok {
+		t.Fatal("absent symbol missing from result map; it must be cached as not-found")
+	}
+	if got.Found {
+		t.Error("absent symbol reported as found")
+	}
+}
+
+func TestLookupSymbolsUnreadableBinaryIsNotCached(t *testing.T) {
+	missing := filepath.Join(t.TempDir(), "does-not-exist")
+	if _, err := LookupSymbols(missing, []string{"main"}); err == nil {
+		t.Fatal("expected error for unreadable binary")
+	}
+	if _, err := binaryKeyFor(missing); err == nil {
+		t.Error("expected stat error for missing file")
+	}
+}
+
+// Distinct binaries must not share an entry.
+func TestBinaryKeyDistinguishesFiles(t *testing.T) {
+	dir := t.TempDir()
+	a := filepath.Join(dir, "a")
+	b := filepath.Join(dir, "b")
+	if err := os.WriteFile(a, []byte("aaaa"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(b, []byte("bbbbbb"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	ka, err := binaryKeyFor(a)
+	if err != nil {
+		t.Fatal(err)
+	}
+	kb, err := binaryKeyFor(b)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ka == kb {
+		t.Fatal("different files produced the same cache key")
+	}
+}