From 3bbab9f83e71a601601d54397783dc1937a8e4d9 Mon Sep 17 00:00:00 2001 From: gecube Date: Tue, 30 Jun 2026 15:43:01 +0200 Subject: [PATCH] cache binaries that have nothing to instrument AcquireGlobalUprobe only cached successful attachments. When attach() returned no links (e.g. a stripped Go binary such as kubectl, which has no symbol table), nothing was remembered, so the binary was re-opened and its symbols re-parsed for every new process that ran it. On nodes that run such binaries frequently this is pure overhead. Cache the negative result in a separate map keyed by dev+inode. The binary's size and mtime are stored in the entry and re-checked on every lookup, so a binary replaced in place or whose inode is recycled is re-evaluated rather than served a stale verdict. Entries expire after a TTL; expired ones are swept on insertion, but at most once per TTL, so a burst of new binaries does not turn every insertion into a full-map scan under the lock. Live (refcounted) attachments are unchanged. --- ebpftracer/tracer.go | 57 ++++++++++++- ebpftracer/tracer_cache_test.go | 147 ++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 ebpftracer/tracer_cache_test.go diff --git a/ebpftracer/tracer.go b/ebpftracer/tracer.go index 71db366..c5be886 100644 --- a/ebpftracer/tracer.go +++ b/ebpftracer/tracer.go @@ -91,6 +91,25 @@ type globalUprobe struct { refcount int } +// negativeUprobe records that a binary had nothing to instrument (e.g. a +// stripped Go binary such as kubectl), so it is not re-opened and re-parsed for +// every process that runs it. It is keyed by dev+inode; size and mtime are kept +// in the value and re-checked on every lookup so an in-place replacement or a +// recycled inode is re-evaluated instead of being served a stale verdict. The +// entry also self-expires after negativeUprobeCacheTTL. This is a heuristic, not +// tamper-proof: a replacement that preserves both size and mtime within the +// filesystem's timestamp granularity is only re-evaluated once the TTL elapses. +type negativeUprobe struct { + size int64 + mtime int64 + expiresAt time.Time +} + +// negativeUprobeCacheTTL bounds how long a "nothing to instrument" verdict is +// trusted, so a binary that is later replaced in a way size+mtime cannot detect +// is still eventually re-evaluated. A var (not const) so tests can shorten it. +var negativeUprobeCacheTTL = 10 * time.Minute + type Tracer struct { disableL7Tracing bool hostNetNs netns.NsHandle @@ -103,7 +122,10 @@ type Tracer struct { uprobes map[string]*ebpf.Program globalUprobes map[UprobeKey]*globalUprobe + negativeUprobes map[UprobeKey]negativeUprobe + negativeSweepAt time.Time globalUprobesLock sync.Mutex + now func() time.Time // overridable in tests } func NewTracer(hostNetNs, selfNetNs netns.NsHandle, disableL7Tracing bool) *Tracer { @@ -115,9 +137,11 @@ func NewTracer(hostNetNs, selfNetNs netns.NsHandle, disableL7Tracing bool) *Trac hostNetNs: hostNetNs, selfNetNs: selfNetNs, - readers: map[string]*perf.Reader{}, - uprobes: map[string]*ebpf.Program{}, - globalUprobes: map[UprobeKey]*globalUprobe{}, + readers: map[string]*perf.Reader{}, + uprobes: map[string]*ebpf.Program{}, + globalUprobes: map[UprobeKey]*globalUprobe{}, + negativeUprobes: map[UprobeKey]negativeUprobe{}, + now: time.Now, } } @@ -154,6 +178,7 @@ func (t *Tracer) Close() { } } t.globalUprobes = nil + t.negativeUprobes = nil t.globalUprobesLock.Unlock() t.collection.Close() } @@ -164,6 +189,7 @@ func (t *Tracer) AcquireGlobalUprobe(path string, attach func() []link.Link) (Up return UprobeKey{}, false } key := UprobeKey{Dev: stat.Dev, Ino: stat.Ino} + size, mtime := stat.Size, stat.Mtim.Nano() t.globalUprobesLock.Lock() defer t.globalUprobesLock.Unlock() @@ -172,14 +198,39 @@ func (t *Tracer) AcquireGlobalUprobe(path string, attach func() []link.Link) (Up gu.refcount++ return key, true } + if neg, ok := t.negativeUprobes[key]; ok { + if t.now().Before(neg.expiresAt) && neg.size == size && neg.mtime == mtime { + return UprobeKey{}, false // known-empty, unchanged, not expired + } + delete(t.negativeUprobes, key) // expired or replaced: re-evaluate below + } links := attach() if len(links) == 0 { + t.cacheNegativeUprobe(key, size, mtime) return UprobeKey{}, false } t.globalUprobes[key] = &globalUprobe{links: links, refcount: 1} return key, true } +// cacheNegativeUprobe remembers that key has nothing to instrument. Expired +// entries are swept here, but at most once per TTL (negativeSweepAt), so a burst +// of distinct binaries does not turn every insert into a full-map scan under the +// lock. Stale entries are also dropped lazily on lookup in AcquireGlobalUprobe. +// The caller must hold globalUprobesLock. +func (t *Tracer) cacheNegativeUprobe(key UprobeKey, size, mtime int64) { + now := t.now() + if now.Sub(t.negativeSweepAt) >= negativeUprobeCacheTTL { + for k, neg := range t.negativeUprobes { + if now.After(neg.expiresAt) { + delete(t.negativeUprobes, k) + } + } + t.negativeSweepAt = now + } + t.negativeUprobes[key] = negativeUprobe{size: size, mtime: mtime, expiresAt: now.Add(negativeUprobeCacheTTL)} +} + func (t *Tracer) ReleaseGlobalUprobes(keys ...UprobeKey) { t.globalUprobesLock.Lock() defer t.globalUprobesLock.Unlock() diff --git a/ebpftracer/tracer_cache_test.go b/ebpftracer/tracer_cache_test.go new file mode 100644 index 0000000..a45c0d6 --- /dev/null +++ b/ebpftracer/tracer_cache_test.go @@ -0,0 +1,147 @@ +package ebpftracer + +import ( + "os" + "path/filepath" + "strconv" + "sync" + "testing" + "time" + + "github.com/cilium/ebpf/link" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestTracer() *Tracer { + return &Tracer{ + globalUprobes: map[UprobeKey]*globalUprobe{}, + negativeUprobes: map[UprobeKey]negativeUprobe{}, + now: time.Now, + } +} + +func writeFile(t *testing.T, content string) string { + t.Helper() + p := filepath.Join(t.TempDir(), "bin") + require.NoError(t, os.WriteFile(p, []byte(content), 0755)) + return p +} + +// A binary with nothing to instrument must be parsed once and then served from +// the cache, instead of being re-opened for every process that runs it. +func TestAcquireGlobalUprobeCachesEmptyResult(t *testing.T) { + tr := newTestTracer() + path := writeFile(t, "binary") + + calls := 0 + attach := func() []link.Link { calls++; return nil } + + for i := 0; i < 5; i++ { + key, ok := tr.AcquireGlobalUprobe(path, attach) + assert.False(t, ok) + assert.Equal(t, UprobeKey{}, key) + } + assert.Equal(t, 1, calls, "attach must be called only once for an unchanged binary") +} + +// A replaced binary (different size/mtime) must be evaluated again rather than +// reusing a stale negative result. +func TestAcquireGlobalUprobeReevaluatesReplacedBinary(t *testing.T) { + tr := newTestTracer() + path := writeFile(t, "v1") + + calls := 0 + attach := func() []link.Link { calls++; return nil } + + _, ok := tr.AcquireGlobalUprobe(path, attach) + require.False(t, ok) + require.Equal(t, 1, calls) + + require.NoError(t, os.WriteFile(path, []byte("v2-longer"), 0755)) + _, ok = tr.AcquireGlobalUprobe(path, attach) + require.False(t, ok) + assert.Equal(t, 2, calls, "a replaced binary must be re-evaluated") +} + +// A binary repeatedly overwritten in place keeps the same inode, so the negative +// cache must hold exactly one entry, not one per spawn. +func TestNegativeCacheDoesNotGrowOnInPlaceOverwrite(t *testing.T) { + tr := newTestTracer() + path := writeFile(t, "v1") + attach := func() []link.Link { return nil } + + for i := 0; i < 20; i++ { + require.NoError(t, os.WriteFile(path, []byte("v"+strconv.Itoa(i)), 0755)) + _, ok := tr.AcquireGlobalUprobe(path, attach) + require.False(t, ok) + } + assert.Len(t, tr.negativeUprobes, 1, "in-place overwrite must reuse one inode-keyed slot") +} + +// An expired negative entry must be re-evaluated and replaced, not duplicated. +// Expiry is driven by an injected clock rather than by poking internals. +func TestAcquireGlobalUprobeExpiresEmptyResult(t *testing.T) { + tr := newTestTracer() + clock := time.Unix(0, 0) + tr.now = func() time.Time { return clock } + path := writeFile(t, "binary") + + calls := 0 + attach := func() []link.Link { calls++; return nil } + + _, ok := tr.AcquireGlobalUprobe(path, attach) + require.False(t, ok) + require.Equal(t, 1, calls) + require.Len(t, tr.negativeUprobes, 1) + + clock = clock.Add(negativeUprobeCacheTTL + time.Second) + _, ok = tr.AcquireGlobalUprobe(path, attach) + require.False(t, ok) + assert.Equal(t, 2, calls, "an expired negative entry must be re-evaluated") + assert.Len(t, tr.negativeUprobes, 1, "the stale entry must be replaced, not duplicated") +} + +// Expired negative entries must not accumulate; they are swept when a new +// negative entry is added (at most once per TTL). +func TestNegativeCacheSweepsExpiredEntries(t *testing.T) { + tr := newTestTracer() + clock := time.Unix(0, 0) + tr.now = func() time.Time { return clock } + + for i := 0; i < 10; i++ { + tr.negativeUprobes[UprobeKey{Ino: uint64(i + 1)}] = negativeUprobe{ + expiresAt: clock.Add(time.Minute), + } + } + // A live (refcounted) entry must be untouched by the negative-cache sweep. + tr.globalUprobes[UprobeKey{Ino: 100}] = &globalUprobe{refcount: 1} + + clock = clock.Add(negativeUprobeCacheTTL + time.Minute) + _, ok := tr.AcquireGlobalUprobe(writeFile(t, "binary"), func() []link.Link { return nil }) + require.False(t, ok) + + assert.Len(t, tr.negativeUprobes, 1, "expired negative entries must be swept; only the fresh one remains") + require.Len(t, tr.globalUprobes, 1, "live entries must be kept") + _, live := tr.globalUprobes[UprobeKey{Ino: 100}] + assert.True(t, live) +} + +// The race detector must stay clean under concurrent acquisition of the same +// binary, and the result must be a single negative entry. +func TestNegativeCacheConcurrent(t *testing.T) { + tr := newTestTracer() + path := writeFile(t, "binary") + attach := func() []link.Link { return nil } + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + tr.AcquireGlobalUprobe(path, attach) + }() + } + wg.Wait() + assert.Len(t, tr.negativeUprobes, 1) +}