From 18960a5dad6a64010f620eb0f70285e97cbff57c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Wed, 12 Aug 2026 12:30:36 +0000 Subject: [PATCH 1/4] adapter/statsclient: refresh symlink entries in UpdateDir updateStatOnIndex skips an entry whose directory type no longer matches the type recorded at PrepareDir. For a symlink those never match: the directory type stays Symlink while entry.Type is the resolved type of the counter it aliases. So every symlink in a prepared dir was silently left at its PrepareDir value, and a PrepareDir-once + UpdateDir-per-tick loop over, say, "/interfaces" reported the same numbers forever. Re-resolve symlinks through CopyEntryData instead. That allocates, where the non-symlink path updates in place, because a resolved item has no stable backing slice to write into - noted in a comment so callers refreshing large numbers of symlinks know to expect it. Adds a synthetic v2 stats segment to test against, laid out as VPP lays out the real one, so the refresh can be shown to pick up a changed backing counter without needing a running VPP to generate traffic. Co-Authored-By: Claude Opus 5 (1M context) --- adapter/statsclient/statsclient.go | 21 +- adapter/statsclient/statseg_v2_fake_test.go | 267 ++++++++++++++++++++ 2 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 adapter/statsclient/statseg_v2_fake_test.go diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index 90a59587..a5331ccc 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -1,4 +1,5 @@ // Copyright (c) 2019 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -629,10 +630,22 @@ func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVec return fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen) } dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index) - if len(dirName) == 0 || - !bytes.Equal(dirName, entry.Name) || - dirType != entry.Type || - entry.Data == nil { + // Identity is the name; if it no longer matches, the directory changed under us + // (the epoch check in UpdateDir normally catches this first). + if len(dirName) == 0 || !bytes.Equal(dirName, entry.Name) || entry.Data == nil { + return nil + } + if dirType == adapter.Symlink { + // A symlink's directory entry holds (target, item) indexes rather than a data + // pointer, so its resolved Type never equals dirType and the type check below + // would skip it, leaving the entry frozen at its PrepareDir value forever. + // Re-resolve through the symlink instead. This allocates, unlike the in-place + // UpdateEntryData path, because the resolved item does not have a stable + // backing slice to write into. + entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0)) + return nil + } + if dirType != entry.Type { return nil } if err := sc.UpdateEntryData(dirPtr, &entry.Data); err != nil { diff --git a/adapter/statsclient/statseg_v2_fake_test.go b/adapter/statsclient/statseg_v2_fake_test.go new file mode 100644 index 00000000..dd0cca8d --- /dev/null +++ b/adapter/statsclient/statseg_v2_fake_test.go @@ -0,0 +1,267 @@ +// Copyright (c) 2026 Meter, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package statsclient + +import ( + "sync/atomic" + "testing" + "unsafe" + + "go.fd.io/govpp/adapter" +) + +// A synthetic v2 stats segment, laid out exactly as VPP lays out the real one, so +// the client's unsafe pointer walking can be exercised without a running VPP. It +// mirrors the shape VPP uses for error counters - one vector, plus a symlink naming +// each of its items: +// +// index 0: /sys/fake-scalar filler, see fakeTargetIndex +// index 1: /node/errors simple counter vector, one thread +// index 2+: /err/fake-node/ symlinks, one per item of that vector +// +// Pointers stored inside the segment are VPP-side addresses (fakeBase + offset), +// which is what adjust() expects to translate back into the mapped region. +const fakeBase = uint64(0x7f0000000000) + +// v2 stat segment directory types, per dirTypeMapping. +const ( + fakeTypeScalarIndex = 1 + fakeTypeSimpleCounterVector = 2 + fakeTypeSymlink = 6 +) + +// fakeTargetIndex is the directory index of /node/errors. It is deliberately not 0: +// CopyEntryData treats a directory entry whose union data is zero as having no data, +// so a symlink to item 0 of directory index 0 is unrepresentable. Real VPP never +// lands there either, but a fake that did would fail for that reason alone. +const fakeTargetIndex = 1 + +// shared header field offsets, per sharedHeaderV2. +const ( + fakeOffVersion = 0 + fakeOffBase = 8 + fakeOffEpoch = 16 + fakeOffInProgress = 24 + fakeOffDirVector = 32 + fakeOffErrorVector = 40 +) + +type fakeSegment struct { + buf []byte + // counters is the offset of the backing counter data for thread 0. + counters int +} + +// newFakeSegment builds a segment holding a /node/errors vector with the given +// counter values, plus one /err/fake-node/rN symlink per value, in reverse order so +// that a symlink's own directory index is never its item index (which would let an +// off-by-one confusion pass unnoticed). +func newFakeSegment(t *testing.T, values []uint64) *fakeSegment { + t.Helper() + + const ( + hdrSize = 64 // sharedHeaderV2 rounded up + vecHdr = 8 // vector length precedes the data + ptrSize = 8 + threads = 1 + trailer = 8 // adjust() rejects pointers to the very last byte + dirEntLen = int(unsafe.Sizeof(statSegDirectoryEntryV2{})) + ) + nDir := fakeTargetIndex + 1 + len(values) // filler + /node/errors + one symlink per value + + dirLenOff := hdrSize + dirOff := dirLenOff + vecHdr + ptLenOff := dirOff + nDir*dirEntLen // per-thread vector of pointers + ptOff := ptLenOff + vecHdr + ctLenOff := ptOff + threads*ptrSize // thread 0 counter vector + ctOff := ctLenOff + vecHdr + total := ctOff + len(values)*ptrSize + trailer + + f := &fakeSegment{buf: make([]byte, total), counters: ctOff} + + // Shared header. errorVector stays zero: adjust() then rejects it, which is how + // the client decides a segment uses the modern (non-legacy) type mapping. + f.putU64(fakeOffVersion, 2) + f.putU64(fakeOffBase, fakeBase) + f.putU64(fakeOffEpoch, 1) + f.putU64(fakeOffInProgress, 0) + f.putU64(fakeOffDirVector, fakeBase+uint64(dirOff)) + f.putU64(fakeOffErrorVector, 0) + + // Vector lengths. + f.putU64(dirLenOff, uint64(nDir)) + f.putU64(ptLenOff, threads) + f.putU64(ctLenOff, uint64(len(values))) + + // Counter data, and the per-thread vector pointing at it. + f.putU64(ptOff, fakeBase+uint64(ctOff)) + for i, v := range values { + f.putU64(ctOff+i*ptrSize, v) + } + + // Directory entry 0: filler, so the backing vector is not at index 0. + f.putDirEntry(dirOff, 0, fakeTypeScalarIndex, 7, "/sys/fake-scalar") + + // Directory entry 1: the backing vector. + f.putDirEntry(dirOff, fakeTargetIndex, fakeTypeSimpleCounterVector, fakeBase+uint64(ptOff), "/node/errors") + + // The remaining entries are symlinks into it, named in reverse item order. + for i := range values { + item := uint32(len(values) - 1 - i) + union := uint64(fakeTargetIndex) | uint64(item)<<32 + f.putDirEntry(dirOff, fakeTargetIndex+1+i, fakeTypeSymlink, union, fakeErrName(item)) + } + return f +} + +func fakeErrName(item uint32) string { + return "/err/fake-node/r" + string(rune('a'+item)) +} + +// fakeErrItem is the inverse of fakeErrName, so a test can tell which counter a +// symlink entry should be showing without relying on the API under test. +func fakeErrItem(t *testing.T, name []byte) uint32 { + t.Helper() + for item := uint32(0); item < 32; item++ { + if fakeErrName(item) == string(name) { + return item + } + } + t.Fatalf("%s is not a fake symlink name", name) + return 0 +} + +func (f *fakeSegment) putU64(off int, v uint64) { + *(*uint64)(unsafe.Pointer(&f.buf[off])) = v +} + +func (f *fakeSegment) putDirEntry(dirOff, index int, typ dirType, union uint64, name string) { + e := (*statSegDirectoryEntryV2)(unsafe.Pointer(&f.buf[dirOff+index*int(unsafe.Sizeof(statSegDirectoryEntryV2{}))])) + e.directoryType = typ + e.unionData = union + copy(e.name[:], name) + e.name[len(name)] = 0 +} + +// setCounter changes a backing counter value, as VPP would between two reads. +func (f *fakeSegment) setCounter(item int, v uint64) { + f.putU64(f.counters+item*8, v) +} + +// bumpEpoch simulates a directory re-layout. +func (f *fakeSegment) bumpEpoch() { + f.putU64(fakeOffEpoch, *(*uint64)(unsafe.Pointer(&f.buf[fakeOffEpoch]))+1) +} + +// client returns a StatsClient reading this segment, without a socket. +func (f *fakeSegment) client() *StatsClient { + sc := &StatsClient{statSegment: newStatSegmentV2(f.buf, int64(len(f.buf)))} + atomic.StoreUint32(&sc.connected, 1) + return sc +} + +// symlinkValue returns the single counter value an entry resolved through a symlink +// carries. +func symlinkValue(t *testing.T, e adapter.StatEntry) uint64 { + t.Helper() + s, ok := e.Data.(adapter.SimpleCounterStat) + if !ok { + t.Fatalf("%s: expected SimpleCounterStat, got %T", e.Name, e.Data) + } + if len(s) != 1 || len(s[0]) != 1 { + t.Fatalf("%s: expected a single resolved item, got %v", e.Name, s) + } + return uint64(s[0][0]) +} + +// UpdateDir must re-resolve symlink entries. Before the fix the type check in +// updateStatOnIndex skipped them - a symlink's directory type never equals the +// resolved type of its data - so a prepared dir kept returning its PrepareDir values. +func TestUpdateDirRefreshesSymlinks(t *testing.T) { + values := []uint64{1, 2, 3} + f := newFakeSegment(t, values) + sc := f.client() + + dir, err := sc.PrepareDir() + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + + // Change every backing counter, exactly as VPP would while counting. + updated := []uint64{100, 200, 300} + for i, v := range updated { + f.setCounter(i, v) + } + + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + var seen int + for i := range dir.Entries { + e := dir.Entries[i] + if !e.Symlink { + continue + } + seen++ + item := fakeErrItem(t, e.Name) + if got, want := symlinkValue(t, e), updated[item]; got != want { + t.Errorf("%s: value after UpdateDir = %d, want %d (stale value was %d)", + e.Name, got, want, values[item]) + } + } + if seen != len(values) { + t.Fatalf("expected %d symlink entries in the prepared dir, got %d", len(values), seen) + } +} + +// The non-symlink path must keep working, in place, as before. +func TestUpdateDirRefreshesCounterVector(t *testing.T) { + f := newFakeSegment(t, []uint64{1, 2, 3}) + sc := f.client() + + dir, err := sc.PrepareDir("^/node/errors$") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + if len(dir.Entries) != 1 { + t.Fatalf("expected one entry, got %d", len(dir.Entries)) + } + f.setCounter(1, 42) + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + s, ok := dir.Entries[0].Data.(adapter.SimpleCounterStat) + if !ok { + t.Fatalf("expected SimpleCounterStat, got %T", dir.Entries[0].Data) + } + if got := uint64(s[0][1]); got != 42 { + t.Errorf("counter after UpdateDir = %d, want 42", got) + } +} + +func TestUpdateDirStaleEpoch(t *testing.T) { + f := newFakeSegment(t, []uint64{1, 2, 3}) + sc := f.client() + + dir, err := sc.PrepareDir() + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + f.bumpEpoch() + if err := sc.UpdateDir(dir); err != adapter.ErrStatsDirStale { + t.Fatalf("UpdateDir after epoch change = %v, want %v", err, adapter.ErrStatsDirStale) + } +} From e1718f5295fa589d43a3e56939429fe2aa6d5547 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Wed, 12 Aug 2026 12:30:59 +0000 Subject: [PATCH 2/4] adapter/statsclient: check the prepared dir against the epoch access starts on UpdateDir read the epoch once for the staleness check and then let accessStart read it again. If the directory is re-laid-out between the two reads, the staleness check passes against the old epoch while the entries are resolved against the new directory - and accessEnd then confirms that same new epoch, so nothing catches it and the caller gets values read against a directory its entry indexes no longer describe. Drop the separate read and compare dir.Epoch against the epoch accessStart settled on, which is the one accessEnd validates. Also return an error when the directory vector is nil, rather than the nil named return, which reported success. Co-Authored-By: Claude Opus 5 (1M context) --- adapter/statsclient/statsclient.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index a5331ccc..93dd8e20 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -283,18 +283,21 @@ func (sc *StatsClient) UpdateDir(dir *adapter.StatDir) (err error) { return adapter.ErrStatsDisconnected } - epoch, _ := sc.GetEpoch() - if dir.Epoch != epoch { - return adapter.ErrStatsDirStale - } - + // Compare the prepared dir against the epoch accessStart settled on, not against + // a separately read one: with two reads the directory can be re-laid-out in + // between, in which case the staleness check passes against the old epoch while + // the entries are resolved against the new directory - and accessEnd then + // confirms that same new epoch, so nothing catches it. accessEpoch := sc.accessStart() if accessEpoch == 0 { return adapter.ErrStatsAccessFailed } + if dir.Epoch != accessEpoch { + return adapter.ErrStatsDirStale + } dirVector := sc.GetDirectoryVector() if dirVector == nil { - return err + return fmt.Errorf("failed to update dir: directory vector is nil") } for i := 0; i < len(dir.Entries); i++ { if err := sc.updateStatOnIndex(&dir.Entries[i], dirVector); err != nil { From 3cbab7e804af0be7133753b548baf0b3dcf975fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Mon, 17 Aug 2026 08:08:08 +0000 Subject: [PATCH 3/4] adapter/statsclient: read each symlink target once when refreshing a dir VPP exposes some counters only as one vector plus a set of symlinks naming its items. /node/errors is the case that hurts: it is a single counter vector, and every /err// is a symlink into one item of it. A capture from a live device has 4223 such symlinks over a 4223-item vector. UpdateDir resolved each of those individually, and resolving a symlink reads its whole backing vector - so refreshing a prepared dir over that fan re-read the same vector 4223 times. Group the symlinks in a prepared dir by the entry they alias, read each target once, and fan its items out into the prepared entries. The fan-out writes through the slices the entries already hold, so it does not allocate per symlink, and it produces exactly the shape resolving each symlink would: one value per worker thread. Anything the fan-out does not recognise falls back to resolving that symlink on its own, as does segment v1, which has no symlink target encoding. This stays entirely inside statsclient. An earlier version of this change exposed the (target, item) pair as a public ListSymlinks API so a caller could read the vector and label its items itself; doing the grouping here gives the same performance model with no API change at all, as suggested in review. Two allocation sources found on the way, both paid per symlink per refresh: storing the fanned-out slice back into the adapter.Stat interface boxed it, and getSymlinkIndexes serialised the union through a bytes.Buffer to split one uint64 into two uint32s. Refreshing 128 symlinks went from 654 allocations to 142 - the remainder being one name clone per prepared entry. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 18 ++ adapter/statsclient/stat_segment_api.go | 7 + adapter/statsclient/statsclient.go | 142 +++++++++++++-- .../statsclient/statseg_symlink_group_test.go | 171 ++++++++++++++++++ adapter/statsclient/statseg_v1.go | 7 + adapter/statsclient/statseg_v2.go | 38 ++-- adapter/statsclient/statseg_v2_fake_test.go | 7 +- test/integration/stats_test.go | 92 ++++++++++ 8 files changed, 447 insertions(+), 35 deletions(-) create mode 100644 adapter/statsclient/statseg_symlink_group_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 557e7af0..95a86fbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,24 @@ This file lists changes for the GoVPP releases. - --> +## Unreleased + +### Fixes + +- adapter/statsclient: `UpdateDir` now refreshes symlink entries. Their resolved + type never equals the directory type, so the type check skipped them and a + prepared dir kept returning its `PrepareDir` values indefinitely. +- adapter/statsclient: `UpdateDir` compares the prepared dir against the epoch + `accessStart` settled on, closing a window where a directory re-layout between + the two epoch reads went undetected. + +### Other + +- adapter/statsclient: `UpdateDir` resolves symlinks by target group, reading + each aliased counter vector once instead of once per symlink naming it. No API + change; a prepared dir over a large symlink fan simply refreshes far more + cheaply. + ## 0.13.0 > _13 November 2025_ diff --git a/adapter/statsclient/stat_segment_api.go b/adapter/statsclient/stat_segment_api.go index af7ca71b..680c8b3c 100644 --- a/adapter/statsclient/stat_segment_api.go +++ b/adapter/statsclient/stat_segment_api.go @@ -1,4 +1,5 @@ // Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -93,6 +94,12 @@ type statSegment interface { // Use ^uint32(0) as an empty index (since 0 is a valid value). CopyEntryData(segment dirSegment, index uint32) adapter.Stat + // GetSymlinkIndexes returns, for a symlink directory segment, the directory + // index of the entry it aliases and the item index within that entry. + // ok is false if the segment is not a symlink, or if the segment version has + // no notion of symlinks. + GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32, ok bool) + // UpdateEntryData accepts pointer to a directory segment with data, and stat // segment to update UpdateEntryData(segment dirSegment, s *adapter.Stat) error diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index 93dd8e20..3514a5ca 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -299,10 +299,27 @@ func (sc *StatsClient) UpdateDir(dir *adapter.StatDir) (err error) { if dirVector == nil { return fmt.Errorf("failed to update dir: directory vector is nil") } + // Symlinks are not refreshed one by one. VPP names every item of a counter + // vector with its own symlink - /node/errors and its thousands of + // /err// aliases being the case that hurts - so resolving them + // individually re-reads the same backing vector once per item. Collect them + // by target instead and read each target once, below. + var symlinks map[uint32][]symlinkRef for i := 0; i < len(dir.Entries); i++ { - if err := sc.updateStatOnIndex(&dir.Entries[i], dirVector); err != nil { + ref, isSymlink, err := sc.updateStatOnIndex(&dir.Entries[i], dirVector) + if err != nil { return err } + if !isSymlink { + continue + } + if symlinks == nil { + symlinks = make(map[uint32][]symlinkRef) + } + symlinks[ref.target] = append(symlinks[ref.target], ref) + } + if err := sc.updateSymlinkGroups(dirVector, symlinks); err != nil { + return err } if !sc.accessEnd(accessEpoch) { return adapter.ErrStatsDataBusy @@ -627,32 +644,127 @@ func (sc *StatsClient) isConnected() bool { } // updateStatOnIndex refreshes the entry data. -func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVector) (err error) { +// symlinkRef is a prepared symlink entry and the counter it aliases, held until +// its target can be read once for the whole group. +type symlinkRef struct { + entry *adapter.StatEntry + dirPtr dirSegment // the symlink's own directory segment, for the fallback + target uint32 // directory index of the aliased entry + item uint32 // item within that entry +} + +// updateStatOnIndex refreshes one entry. A symlink is not resolved here: it is +// reported to the caller so that every symlink sharing a target can be served +// from a single read of it. +func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVector) (ref symlinkRef, isSymlink bool, err error) { dirLen := *(*uint32)(vectorLen(vector)) if entry.Index >= dirLen { - return fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen) + return ref, false, fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen) } dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index) // Identity is the name; if it no longer matches, the directory changed under us // (the epoch check in UpdateDir normally catches this first). if len(dirName) == 0 || !bytes.Equal(dirName, entry.Name) || entry.Data == nil { - return nil + return ref, false, nil } if dirType == adapter.Symlink { - // A symlink's directory entry holds (target, item) indexes rather than a data - // pointer, so its resolved Type never equals dirType and the type check below - // would skip it, leaving the entry frozen at its PrepareDir value forever. - // Re-resolve through the symlink instead. This allocates, unlike the in-place - // UpdateEntryData path, because the resolved item does not have a stable - // backing slice to write into. - entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0)) - return nil + // A symlink's directory entry holds (target, item) indexes rather than a + // data pointer, so its resolved Type never equals dirType and the type + // check below would skip it, leaving the entry frozen at its PrepareDir + // value forever. + target, item, ok := sc.GetSymlinkIndexes(dirPtr) + if !ok { + // No target encoding (segment v1): resolve it on its own. + entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0)) + return ref, false, nil + } + return symlinkRef{entry: entry, dirPtr: dirPtr, target: target, item: item}, true, nil } if dirType != entry.Type { - return nil + return ref, false, nil } if err := sc.UpdateEntryData(dirPtr, &entry.Data); err != nil { - return fmt.Errorf("updating stat data for entry %s failed: %v", dirName, err) + return ref, false, fmt.Errorf("updating stat data for entry %s failed: %v", dirName, err) + } + return ref, false, nil +} + +// updateSymlinkGroups reads each aliased entry once and fans its items out to +// the symlinks that name them. Reading the target once is the whole point: a +// vector of n items aliased by n symlinks costs one read here rather than n. +func (sc *StatsClient) updateSymlinkGroups(vector dirVector, groups map[uint32][]symlinkRef) error { + if len(groups) == 0 { + return nil + } + dirLen := *(*uint32)(vectorLen(vector)) + for target, refs := range groups { + if target >= dirLen { + debugf("symlink target index %d out of dir vector length (%d)", target, dirLen) + continue + } + targetPtr, targetName, _ := sc.GetStatDirOnIndex(vector, target) + if len(targetName) == 0 { + continue + } + full := sc.CopyEntryData(targetPtr, ^uint32(0)) + for _, ref := range refs { + if !symlinkItem(full, ref.item, &ref.entry.Data) { + // A shape this cannot fan out - resolve the symlink on its own + // so behaviour matches the one-at-a-time path exactly. + ref.entry.Data = sc.CopyEntryData(ref.dirPtr, ^uint32(0)) + } + } + } + return nil +} + +// symlinkItem extracts one item from an already-read counter vector into dst, in +// the same shape resolving the symlink directly would produce: one value per +// worker thread. +// +// It writes THROUGH dst rather than returning a new value, and only stores back +// when the outer slice had to be reallocated. Storing a slice into an +// adapter.Stat boxes it, which allocates - once per symlink per tick, for a +// value that is usually identical to the one already there. +// +// Reports false for anything it cannot fan out, so the caller can fall back. +func symlinkItem(full adapter.Stat, item uint32, dst *adapter.Stat) bool { + switch d := full.(type) { + case adapter.SimpleCounterStat: + out, ok := (*dst).(adapter.SimpleCounterStat) + if !ok || len(out) != len(d) { + out = make(adapter.SimpleCounterStat, len(d)) + *dst = out + } + for i, worker := range d { + if len(out[i]) != 1 { + out[i] = make([]adapter.Counter, 1) + } + if int(item) < len(worker) { + out[i][0] = worker[item] + } else { + out[i][0] = 0 + } + } + return true + + case adapter.CombinedCounterStat: + out, ok := (*dst).(adapter.CombinedCounterStat) + if !ok || len(out) != len(d) { + out = make(adapter.CombinedCounterStat, len(d)) + *dst = out + } + for i, worker := range d { + if len(out[i]) != 1 { + out[i] = make([]adapter.CombinedCounter, 1) + } + if int(item) < len(worker) { + out[i][0] = worker[item] + } else { + out[i][0] = adapter.CombinedCounter{} + } + } + return true } - return + return false } diff --git a/adapter/statsclient/statseg_symlink_group_test.go b/adapter/statsclient/statseg_symlink_group_test.go new file mode 100644 index 00000000..e94bdb6d --- /dev/null +++ b/adapter/statsclient/statseg_symlink_group_test.go @@ -0,0 +1,171 @@ +// Copyright (c) 2026 Meter, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package statsclient + +import ( + "sync/atomic" + "testing" + + "go.fd.io/govpp/adapter" +) + +// countingSegment counts data reads so a test can assert that a symlink fan +// costs ONE read of its target rather than one per symlink. +type countingSegment struct { + statSegment + copies int64 +} + +func (c *countingSegment) CopyEntryData(segment dirSegment, index uint32) adapter.Stat { + atomic.AddInt64(&c.copies, 1) + return c.statSegment.CopyEntryData(segment, index) +} + +func countingClient(f *fakeSegment) (*StatsClient, *countingSegment) { + sc := f.client() + c := &countingSegment{statSegment: sc.statSegment} + sc.statSegment = c + return sc, c +} + +// TestUpdateDirReadsSymlinkTargetOnce is the property the whole grouping exists +// for. VPP names every item of /node/errors with its own /err// +// symlink, so resolving them one at a time re-reads the same backing vector once +// per item - thousands of times per refresh on a real box. +func TestUpdateDirReadsSymlinkTargetOnce(t *testing.T) { + const n = 64 + f := newFakeSegment(t, fakeGroupValues(n)) + sc, counter := countingClient(f) + + dir, err := sc.PrepareDir("^/err/") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + if len(dir.Entries) != n { + t.Fatalf("prepared %d entries, want %d symlinks", len(dir.Entries), n) + } + + atomic.StoreInt64(&counter.copies, 0) + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + // All n symlinks alias the same vector, so one read serves them all. + if got := atomic.LoadInt64(&counter.copies); got != 1 { + t.Errorf("refreshing %d symlinks over one target did %d data reads, want 1", n, got) + } +} + +// TestUpdateDirGroupedMatchesIndividual pins the fan-out against the +// one-at-a-time resolution it replaces: same values, same shape. An off-by-one +// here would mislabel every error counter while looking entirely plausible. +func TestUpdateDirGroupedMatchesIndividual(t *testing.T) { + values := fakeGroupValues(24) + f := newFakeSegment(t, values) + sc := f.client() + + dir, err := sc.PrepareDir("^/err/") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + // Move the counters so the refresh has to do real work. + for i := range values { + values[i] += 7777 + f.setCounter(i, values[i]) + } + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + // Reference: resolve each symlink on its own, as DumpStats does. + individual, err := sc.DumpStats("^/err/") + if err != nil { + t.Fatal("DumpStats failed:", err) + } + want := make(map[string]uint64, len(individual)) + for _, e := range individual { + want[string(e.Name)] = symlinkValue(t, e) + } + + for _, e := range dir.Entries { + got := symlinkValue(t, e) + if w, ok := want[string(e.Name)]; !ok { + t.Errorf("%s: not returned by DumpStats", e.Name) + } else if got != w { + t.Errorf("%s: grouped refresh read %d, individual resolution %d", e.Name, got, w) + } + if item := fakeErrItem(t, e.Name); got != values[item] { + t.Errorf("%s: read %d, backing item %d holds %d", e.Name, got, item, values[item]) + } + } +} + +// TestUpdateDirSymlinkRefreshDoesNotAllocate — the fan-out writes into the +// slices the prepared entries already hold. +func TestUpdateDirSymlinkRefreshDoesNotAllocate(t *testing.T) { + f := newFakeSegment(t, fakeGroupValues(128)) + sc := f.client() + + dir, err := sc.PrepareDir("^/err/") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + if err := sc.UpdateDir(dir); err != nil { // warm up + t.Fatal(err) + } + allocs := testing.AllocsPerRun(20, func() { + if err := sc.UpdateDir(dir); err != nil { + t.Fatal(err) + } + }) + // What remains is one name clone per prepared entry, from GetStatDirOnIndex + // in updateStatOnIndex, plus one read of the target vector. The fan-out + // itself must add nothing per symlink: it writes into the slices the + // prepared entries already hold. + const n = 128 + if allocs > n+40 { + t.Errorf("refreshing %d symlinks allocated %.0f times, want at most one per entry "+ + "(the name clone) plus the target read; the fan-out is not reusing the prepared slices", + n, allocs) + } + t.Logf("allocations per refresh of %d symlinks: %.0f", n, allocs) +} + +func fakeGroupValues(n int) []uint64 { + v := make([]uint64, n) + for i := range v { + v[i] = uint64(i)*13 + 500 + } + return v +} + +// BenchmarkUpdateDirSymlinkFan measures the refresh at the fan size a real box +// carries: a capture from a live device has 4223 /err/* symlinks over a +// 4223-item /node/errors. +func BenchmarkUpdateDirSymlinkFan(b *testing.B) { + f := newFakeSegment(b, fakeGroupValues(4223)) + sc := f.client() + dir, err := sc.PrepareDir("^/err/") + if err != nil { + b.Fatal(err) + } + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := sc.UpdateDir(dir); err != nil { + b.Fatal(err) + } + } +} diff --git a/adapter/statsclient/statseg_v1.go b/adapter/statsclient/statseg_v1.go index 134104b3..b5d3f7ac 100644 --- a/adapter/statsclient/statseg_v1.go +++ b/adapter/statsclient/statseg_v1.go @@ -1,4 +1,5 @@ // Copyright (c) 2019 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -92,6 +93,12 @@ func (ss *statSegmentV1) GetEpoch() (int64, bool) { return sh.epoch, sh.inProgress != 0 } +// GetSymlinkIndexes is unsupported for stats segment v1, which does not encode +// symlink target indexes. +func (ss *statSegmentV1) GetSymlinkIndexes(dirSegment) (uint32, uint32, bool) { + return 0, 0, false +} + func (ss *statSegmentV1) CopyEntryData(segment dirSegment, _ uint32) adapter.Stat { dirEntry := (*statSegDirectoryEntryV1)(segment) typ := getStatType(dirEntry.directoryType, true) diff --git a/adapter/statsclient/statseg_v2.go b/adapter/statsclient/statseg_v2.go index 01bd5f70..52f153f2 100644 --- a/adapter/statsclient/statseg_v2.go +++ b/adapter/statsclient/statseg_v2.go @@ -1,4 +1,5 @@ // Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,7 +17,6 @@ package statsclient import ( "bytes" - "encoding/binary" "sync/atomic" "unsafe" @@ -580,21 +580,25 @@ func (ss *statSegmentV2) getErrorVector() dirVector { return ss.adjust(dirVector(&header.errorVector)) } -func (ss *statSegmentV2) getSymlinkIndexes(dirEntry *statSegDirectoryEntryV2) (index1, index2 uint32) { - var b bytes.Buffer - if err := binary.Write(&b, binary.LittleEndian, dirEntry.unionData); err != nil { - debugf("error getting symlink indexes for %s: %v", dirEntry.name, err) - return - } - if len(b.Bytes()) != 8 { - debugf("incorrect symlink union data length for %s: expected 8, got %d", dirEntry.name, len(b.Bytes())) - return - } - for i := range b.Bytes()[:4] { - index1 += uint32(b.Bytes()[i]) << (uint32(i) * 8) - } - for i := range b.Bytes()[4:] { - index2 += uint32(b.Bytes()[i+4]) << (uint32(i) * 8) +// GetSymlinkIndexes returns the target directory index and item index encoded in a +// symlink directory segment's union data, or ok false if the segment is not a symlink. +func (ss *statSegmentV2) GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32, ok bool) { + dirEntry := (*statSegDirectoryEntryV2)(segment) + if getStatType(dirEntry.directoryType, ss.getErrorVector() != nil) != adapter.Symlink { + return 0, 0, false } - return + targetIndex, itemIndex = ss.getSymlinkIndexes(dirEntry) + return targetIndex, itemIndex, true +} + +func (ss *statSegmentV2) getSymlinkIndexes(dirEntry *statSegDirectoryEntryV2) (index1, index2 uint32) { + // The union holds the two indexes packed into one uint64, low half first. + // Serialising it through a bytes.Buffer to take them apart allocated three + // times per call - which UpdateDir now pays once per prepared symlink per + // refresh, thousands of times on a real box. + // + // unionData is read as a host-order uint64, and the old code wrote it out + // little-endian and reassembled it little-endian, so it round-tripped to the + // same numeric value on any host. Shifting does the same, without the buffer. + return uint32(dirEntry.unionData), uint32(dirEntry.unionData >> 32) } diff --git a/adapter/statsclient/statseg_v2_fake_test.go b/adapter/statsclient/statseg_v2_fake_test.go index dd0cca8d..5891e155 100644 --- a/adapter/statsclient/statseg_v2_fake_test.go +++ b/adapter/statsclient/statseg_v2_fake_test.go @@ -68,7 +68,8 @@ type fakeSegment struct { // counter values, plus one /err/fake-node/rN symlink per value, in reverse order so // that a symlink's own directory index is never its item index (which would let an // off-by-one confusion pass unnoticed). -func newFakeSegment(t *testing.T, values []uint64) *fakeSegment { +// Takes testing.TB so benchmarks can build a segment too. +func newFakeSegment(t testing.TB, values []uint64) *fakeSegment { t.Helper() const ( @@ -132,7 +133,7 @@ func fakeErrName(item uint32) string { // fakeErrItem is the inverse of fakeErrName, so a test can tell which counter a // symlink entry should be showing without relying on the API under test. -func fakeErrItem(t *testing.T, name []byte) uint32 { +func fakeErrItem(t testing.TB, name []byte) uint32 { t.Helper() for item := uint32(0); item < 32; item++ { if fakeErrName(item) == string(name) { @@ -174,7 +175,7 @@ func (f *fakeSegment) client() *StatsClient { // symlinkValue returns the single counter value an entry resolved through a symlink // carries. -func symlinkValue(t *testing.T, e adapter.StatEntry) uint64 { +func symlinkValue(t testing.TB, e adapter.StatEntry) uint64 { t.Helper() s, ok := e.Data.(adapter.SimpleCounterStat) if !ok { diff --git a/test/integration/stats_test.go b/test/integration/stats_test.go index ffa19157..60b3a985 100644 --- a/test/integration/stats_test.go +++ b/test/integration/stats_test.go @@ -1,4 +1,5 @@ // Copyright (c) 2022 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -17,6 +18,8 @@ package integration import ( "testing" + "go.fd.io/govpp/adapter" + "go.fd.io/govpp/adapter/statsclient" "go.fd.io/govpp/api" "go.fd.io/govpp/test/vpptesting" ) @@ -97,3 +100,92 @@ func TestStatClientNodeStatsAgain(t *testing.T) { t.Fatal("getting node stats failed:", err) } } + +// TestStatClientSymlinkRefresh checks that refreshing a prepared dir over a +// symlink fan agrees with resolving each symlink individually, against a real +// VPP directory. +// +// UpdateDir groups symlinks by the entry they alias and reads each target once, +// rather than resolving every symlink separately. /err// is the +// case that motivates it: every one is a symlink into a single /node/errors +// vector, and an item's position there comes from a heap allocation in +// vlib_register_errors, so the grouping has to recover it from the directory +// rather than compute it. +// +// The unit tests in adapter/statsclient cover the pointer walking and the +// fan-out deterministically against a synthetic segment; this test is about +// agreeing with a real VPP's layout. +func TestStatClientSymlinkRefresh(t *testing.T) { + test := vpptesting.SetupVPP(t) + + // Create an interface so the directory carries per-interface symlink entries + // (/interfaces/* aliasing into /if/*) alongside the /err/* ones. + test.MustCli("create loopback interface", "set interface state loop0 up") + + client := statsclient.NewStatsClient("") + if err := client.Connect(); err != nil { + t.Fatal("connecting stats client failed:", err) + } + defer func() { _ = client.Disconnect() }() + + const pattern = "^/err/" + + dir, err := client.PrepareDir(pattern) + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + if len(dir.Entries) == 0 { + t.Skip("no /err/ symlinks in this VPP's stats directory") + } + if err := client.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + // Reference: resolve each symlink on its own, which is what DumpStats does. + individual, err := client.DumpStats(pattern) + if err != nil { + t.Fatal("DumpStats failed:", err) + } + want := make(map[string]adapter.Stat, len(individual)) + for _, e := range individual { + want[string(e.Name)] = e.Data + } + + var checked int + for _, e := range dir.Entries { + w, ok := want[string(e.Name)] + if !ok { + t.Errorf("%s: refreshed by UpdateDir but not returned by DumpStats", e.Name) + continue + } + got, gotOK := e.Data.(adapter.SimpleCounterStat) + exp, expOK := w.(adapter.SimpleCounterStat) + if !gotOK || !expOK { + // Older segments report error counters as ErrorStat; the shapes are + // compared only where both sides are counter vectors. + continue + } + if len(got) != len(exp) { + t.Errorf("%s: grouped refresh has %d worker rows, individual resolution %d", + e.Name, len(got), len(exp)) + continue + } + for i := range exp { + if len(got[i]) != len(exp[i]) { + t.Errorf("%s: worker %d has %d items, want %d", e.Name, i, len(got[i]), len(exp[i])) + break + } + for j := range exp[i] { + if got[i][j] != exp[i][j] { + t.Errorf("%s: worker %d item %d is %d, individual resolution gives %d", + e.Name, i, j, got[i][j], exp[i][j]) + } + } + } + checked++ + } + if checked == 0 { + t.Fatal("no symlink entries were comparable between the two paths") + } + t.Logf("%d /err/ symlinks agree between grouped refresh and individual resolution", checked) +} From 0d2d0aae500071cb56e1ccc89d70ecb52f8a26d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Mon, 17 Aug 2026 08:20:07 +0000 Subject: [PATCH 4/4] adapter/statsclient: compare entry names in place on refresh updateStatOnIndex confirms a prepared entry is still the one it prepared by comparing its name against the directory. It did that through GetStatDirOnIndex, which clones the name out of the shared memory region - correct for callers that retain it, wasteful here, where the copy is compared and dropped on the next line. UpdateDir runs this once per prepared entry per tick. With the previous commit reading each symlink target once, that clone became the dominant remaining cost of a refresh: a prepared dir over a real box's ~3900 entries cloned ~3900 names every tick. Add StatDirOnIndexMatches, which compares in place and reports whether the name at an index equals the caller's, and use it here. GetStatDirOnIndex keeps cloning, since its callers do retain the name. Refreshing 128 symlinks goes from 142 allocations to 14. Both segment versions implement it. The v1 path had no coverage at all - the fake segment harness is v2-only - so the test builds a directory vector directly for each version and checks that a prefix, an extension, a different name of equal length and an empty name are all rejected, and that a mismatch reports adapter.Unknown rather than a plausible type. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 + adapter/statsclient/stat_segment_api.go | 11 ++ adapter/statsclient/statsclient.go | 13 +- adapter/statsclient/statseg_dirmatch_test.go | 133 +++++++++++++++++++ adapter/statsclient/statseg_v1.go | 21 +++ adapter/statsclient/statseg_v2.go | 21 +++ 6 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 adapter/statsclient/statseg_dirmatch_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 95a86fbd..2a5fbba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ This file lists changes for the GoVPP releases. each aliased counter vector once instead of once per symlink naming it. No API change; a prepared dir over a large symlink fan simply refreshes far more cheaply. +- adapter/statsclient: `UpdateDir` compares each prepared entry's name in place + rather than cloning it out of shared memory to compare and discard. Together + with the above, refreshing 128 symlinks goes from 654 allocations to 14. ## 0.13.0 diff --git a/adapter/statsclient/stat_segment_api.go b/adapter/statsclient/stat_segment_api.go index 680c8b3c..32fb9f5b 100644 --- a/adapter/statsclient/stat_segment_api.go +++ b/adapter/statsclient/stat_segment_api.go @@ -84,6 +84,17 @@ type statSegment interface { // the same memory address as the argument. GetStatDirOnIndex(v dirVector, index uint32) (dirSegment, dirName, adapter.StatType) + // StatDirOnIndexMatches is GetStatDirOnIndex for callers that only need to + // confirm an entry is still the one they prepared. It compares the name in + // place instead of copying it out, which matters on a refresh path: UpdateDir + // runs it once per prepared entry per tick, and cloning a name to compare it + // is an allocation per entry for a value discarded immediately afterwards. + // + // ok reports whether the name at index equals want. The segment pointer is + // returned either way; the StatType is meaningful only when ok is true, and + // is adapter.Unknown otherwise. + StatDirOnIndexMatches(v dirVector, index uint32, want []byte) (dirSegment, adapter.StatType, bool) + // GetEpoch re-loads stats header and returns current epoch //and 'inProgress' value GetEpoch() (int64, bool) diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index 3514a5ca..c026c82f 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -17,7 +17,6 @@ package statsclient import ( - "bytes" "fmt" "net" "os" @@ -661,10 +660,12 @@ func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVec if entry.Index >= dirLen { return ref, false, fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen) } - dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index) - // Identity is the name; if it no longer matches, the directory changed under us - // (the epoch check in UpdateDir normally catches this first). - if len(dirName) == 0 || !bytes.Equal(dirName, entry.Name) || entry.Data == nil { + // Identity is the name; if it no longer matches, the directory changed under + // us (the epoch check in UpdateDir normally catches this first). Compared in + // place: this runs once per prepared entry per tick, and cloning the name to + // compare it would allocate per entry for a value discarded immediately after. + dirPtr, dirType, match := sc.StatDirOnIndexMatches(vector, entry.Index, entry.Name) + if !match || entry.Data == nil { return ref, false, nil } if dirType == adapter.Symlink { @@ -684,7 +685,7 @@ func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVec return ref, false, nil } if err := sc.UpdateEntryData(dirPtr, &entry.Data); err != nil { - return ref, false, fmt.Errorf("updating stat data for entry %s failed: %v", dirName, err) + return ref, false, fmt.Errorf("updating stat data for entry %s failed: %v", entry.Name, err) } return ref, false, nil } diff --git a/adapter/statsclient/statseg_dirmatch_test.go b/adapter/statsclient/statseg_dirmatch_test.go new file mode 100644 index 00000000..dff24ae2 --- /dev/null +++ b/adapter/statsclient/statseg_dirmatch_test.go @@ -0,0 +1,133 @@ +// Copyright (c) 2026 Meter, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package statsclient + +import ( + "testing" + "unsafe" + + "go.fd.io/govpp/adapter" +) + +// dirEntryBuf lays out a directory vector holding the given names, for both +// segment versions. The vector is preceded by its length, as VPP lays it out. +func dirEntryBufV1(t *testing.T, names []string) (dirVector, func()) { + t.Helper() + sz := int(unsafe.Sizeof(statSegDirectoryEntryV1{})) + buf := make([]byte, 8+sz*len(names)) + *(*uint32)(unsafe.Pointer(&buf[0])) = uint32(len(names)) + base := unsafe.Pointer(&buf[8]) + for i, n := range names { + e := (*statSegDirectoryEntryV1)(unsafe.Add(base, i*sz)) + copy(e.name[:], n) + e.name[len(n)] = 0 + e.directoryType = 2 // SimpleCounterVector + } + return dirVector(base), func() { runtimeKeepAlive(buf) } +} + +func dirEntryBufV2(t *testing.T, names []string) (dirVector, func()) { + t.Helper() + sz := int(unsafe.Sizeof(statSegDirectoryEntryV2{})) + buf := make([]byte, 8+sz*len(names)) + *(*uint32)(unsafe.Pointer(&buf[0])) = uint32(len(names)) + base := unsafe.Pointer(&buf[8]) + for i, n := range names { + e := (*statSegDirectoryEntryV2)(unsafe.Add(base, i*sz)) + copy(e.name[:], n) + e.name[len(n)] = 0 + e.directoryType = 2 // SimpleCounterVector + } + return dirVector(base), func() { runtimeKeepAlive(buf) } +} + +func runtimeKeepAlive(b []byte) { _ = b } + +// TestStatDirOnIndexMatches covers both segment versions. v1 is easy to leave +// untested - the fake segment harness is v2-only - and it is exactly where a +// regression would go unnoticed. +func TestStatDirOnIndexMatches(t *testing.T) { + names := []string{"/if/rx", "/node/errors", "/sys/heartbeat"} + + for _, tc := range []struct { + version string + build func(*testing.T, []string) (dirVector, func()) + seg statSegment + }{ + {"v1", dirEntryBufV1, &statSegmentV1{}}, + {"v2", dirEntryBufV2, &statSegmentV2{sharedHeader: make([]byte, 1<<12)}}, + } { + t.Run(tc.version, func(t *testing.T) { + v, keep := tc.build(t, names) + defer keep() + + for i, n := range names { + _, typ, ok := tc.seg.StatDirOnIndexMatches(v, uint32(i), []byte(n)) + if !ok { + t.Errorf("index %d (%q): reported no match", i, n) + } + if typ == adapter.Unknown { + t.Errorf("index %d (%q): matched but type is Unknown", i, n) + } + } + + // A name that is a prefix of the stored one must NOT match: the + // length check is what stops /if/rx matching /if/rx-unicast. + if _, _, ok := tc.seg.StatDirOnIndexMatches(v, 0, []byte("/if/r")); ok { + t.Error("a prefix of the stored name matched") + } + // Nor an extension of it. + if _, _, ok := tc.seg.StatDirOnIndexMatches(v, 0, []byte("/if/rx-unicast")); ok { + t.Error("an extension of the stored name matched") + } + // Wrong entry at the right length. + if _, _, ok := tc.seg.StatDirOnIndexMatches(v, 0, []byte("/if/tx")); ok { + t.Error("a different name of equal length matched") + } + // Empty want against a non-empty entry. + if _, _, ok := tc.seg.StatDirOnIndexMatches(v, 0, nil); ok { + t.Error("an empty name matched a populated entry") + } + // A mismatch must report Unknown rather than a plausible type. + if _, typ, _ := tc.seg.StatDirOnIndexMatches(v, 0, []byte("/if/tx")); typ != adapter.Unknown { + t.Errorf("mismatch reported type %v, want Unknown", typ) + } + }) + } +} + +// TestStatDirOnIndexMatchesAgreesWithGet pins the two accessors together: the +// in-place comparison must accept exactly the names GetStatDirOnIndex reports. +func TestStatDirOnIndexMatchesAgreesWithGet(t *testing.T) { + f := newFakeSegment(t, fakeGroupValues(8)) + sc := f.client() + v := sc.GetDirectoryVector() + if v == nil { + t.Fatal("nil directory vector") + } + for i := uint32(0); i < 10; i++ { + _, name, typ := sc.GetStatDirOnIndex(v, i) + if len(name) == 0 { + continue + } + _, mTyp, ok := sc.StatDirOnIndexMatches(v, i, name) + if !ok { + t.Errorf("index %d: GetStatDirOnIndex says %q, Matches disagrees", i, name) + } + if mTyp != typ { + t.Errorf("index %d (%q): type %v vs %v", i, name, mTyp, typ) + } + } +} diff --git a/adapter/statsclient/statseg_v1.go b/adapter/statsclient/statseg_v1.go index b5d3f7ac..e64bec1b 100644 --- a/adapter/statsclient/statseg_v1.go +++ b/adapter/statsclient/statseg_v1.go @@ -88,6 +88,27 @@ func (ss *statSegmentV1) GetStatDirOnIndex(v dirVector, index uint32) (dirSegmen return statSegDir, name, getStatType(dir.directoryType, true) } +// StatDirOnIndexMatches compares the entry name in place - see the interface. +func (ss *statSegmentV1) StatDirOnIndexMatches(v dirVector, index uint32, want []byte) (dirSegment, adapter.StatType, bool) { + statSegDir := dirSegment(uintptr(v) + uintptr(index)*unsafe.Sizeof(statSegDirectoryEntryV1{})) + dir := (*statSegDirectoryEntryV1)(statSegDir) + n := 0 + for ; n < len(dir.name); n++ { + if dir.name[n] == 0 { + break + } + } + if n == 0 || n != len(want) { + return statSegDir, adapter.Unknown, false + } + for i := 0; i < n; i++ { + if dir.name[i] != want[i] { + return statSegDir, adapter.Unknown, false + } + } + return statSegDir, getStatType(dir.directoryType, true), true +} + func (ss *statSegmentV1) GetEpoch() (int64, bool) { sh := ss.loadSharedHeader(ss.sharedHeader) return sh.epoch, sh.inProgress != 0 diff --git a/adapter/statsclient/statseg_v2.go b/adapter/statsclient/statseg_v2.go index 52f153f2..6c7786d7 100644 --- a/adapter/statsclient/statseg_v2.go +++ b/adapter/statsclient/statseg_v2.go @@ -88,6 +88,27 @@ func (ss *statSegmentV2) GetStatDirOnIndex(v dirVector, index uint32) (dirSegmen return statSegDir, name, getStatType(dir.directoryType, ss.getErrorVector() != nil) } +// StatDirOnIndexMatches compares the entry name in place - see the interface. +func (ss *statSegmentV2) StatDirOnIndexMatches(v dirVector, index uint32, want []byte) (dirSegment, adapter.StatType, bool) { + statSegDir := dirSegment(uintptr(v) + uintptr(index)*unsafe.Sizeof(statSegDirectoryEntryV2{})) + dir := (*statSegDirectoryEntryV2)(statSegDir) + n := 0 + for ; n < len(dir.name); n++ { + if dir.name[n] == 0 { + break + } + } + if n == 0 || n != len(want) { + return statSegDir, adapter.Unknown, false + } + for i := 0; i < n; i++ { + if dir.name[i] != want[i] { + return statSegDir, adapter.Unknown, false + } + } + return statSegDir, getStatType(dir.directoryType, ss.getErrorVector() != nil), true +} + func (ss *statSegmentV2) GetEpoch() (int64, bool) { sh := ss.loadSharedHeader(ss.sharedHeader) return sh.epoch, sh.inProgress != 0