diff --git a/CHANGELOG.md b/CHANGELOG.md index 557e7af0..2a5fbba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,27 @@ 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. +- 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 > _13 November 2025_ diff --git a/adapter/statsclient/stat_segment_api.go b/adapter/statsclient/stat_segment_api.go index af7ca71b..32fb9f5b 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. @@ -83,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) @@ -93,6 +105,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 90a59587..c026c82f 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. @@ -16,7 +17,6 @@ package statsclient import ( - "bytes" "fmt" "net" "os" @@ -282,23 +282,43 @@ 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") + } + // 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 @@ -623,20 +643,129 @@ 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) + } + // 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 { + // 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 } - dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index) - if len(dirName) == 0 || - !bytes.Equal(dirName, entry.Name) || - dirType != entry.Type || - entry.Data == nil { - return nil + if dirType != entry.Type { + 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", entry.Name, 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_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_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..e64bec1b 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. @@ -87,11 +88,38 @@ 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 } +// 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..6c7786d7 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" @@ -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 @@ -580,21 +601,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 new file mode 100644 index 00000000..5891e155 --- /dev/null +++ b/adapter/statsclient/statseg_v2_fake_test.go @@ -0,0 +1,268 @@ +// 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). +// Takes testing.TB so benchmarks can build a segment too. +func newFakeSegment(t testing.TB, 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.TB, 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.TB, 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) + } +} 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) +}