From 91347a96ed691927ed1906f01c11b3587cdfd0a8 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 19 Aug 2026 09:16:18 -0500 Subject: [PATCH 01/13] Metrics: one gate for id validation, and fix the off-by-one valid(), lookup(IdType), name() and rename() each carried their own copy of the same range test, and the copies had drifted. valid() rejected an offset past MAX_SIZE; the other three did not. Since _splitID passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, an id such as 0x0000FFFF indexed well past the end of a blob's 1024 entry arrays once a second blob existed. Ids reaching these accessors come from plugins through the TSStat* API, so they are untrusted. All four now go through Storage::_is_allocated(), which rejects a negative id, an offset no _makeId could have produced, an unallocated blob, and a slot at or past the allocation point. That last comparison also fixes an off-by-one: create() returns the id and then advances, so _cur_off is the next free slot, and the old <= / > tests accepted it. An increment there landed on the slot create() would hand out next, and since create() writes only the name and never the value, the next plugin to call TSStatCreate() received a metric already carrying someone else's count. Nothing depended on the loose bound: end() builds an id at the allocation point that is compared but never dereferenced, iterator::next() keeps the offset in range, and find() returns end() on a miss. --- include/tsutil/Metrics.h | 30 ++++++++++++++++++-- src/tsutil/Metrics.cc | 41 +++++++++++++++------------ src/tsutil/unit_tests/test_Metrics.cc | 24 ++++++++++++++++ 3 files changed, 74 insertions(+), 21 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 40cdef9522f..ffdc97768ab 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -357,12 +357,36 @@ class Metrics return {_cur_blob, _cur_off}; } + /** Whether @a id names a slot that has actually been allocated. + * + * The single gate for every id based accessor. Ids arriving through the @c TSStat* API are + * plugin supplied and untrusted, so three things have to hold: the id is not negative, the + * offset is one @c _makeId could have produced -- the offset field is 16 bits wide but a real + * offset is always below @c MAX_SIZE, so a larger one is malformed rather than merely stale -- + * and the slot has been handed out. Blobs are filled in increasing order and never freed, so + * that last part means either an earlier blob, or below the allocation point in the current one. + */ bool - valid(IdType id) const + _is_allocated(IdType id) const { - auto [blob, entry] = _splitID(id); + if (id < 0) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); - return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); + // The blob comparison is against <= / <, not a test for "not the current blob", because + // addBlob() stores a new blob before advancing _cur_blob: for those two instructions + // _blobs[_cur_blob + 1] is non-null while still holding nothing. Requiring the index to be no + // greater than _cur_blob, and the offset to be below _cur_off in that blob, is correct + // whichever of the two the reader happens to observe first. + return offset < MAX_SIZE && blob_ix <= _cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < _cur_blob || offset < _cur_off); + } + + bool + valid(IdType id) const + { + return _is_allocated(id); } }; diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index c339eea2df8..5d7b7072ace 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -62,8 +62,9 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! // The write below is to _blobs[_cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. release_assert(_cur_blob < MAX_BLOBS - 1); - _blobs[++_cur_blob] = std::move(blob); - _cur_off = 0; + _blobs[_cur_blob + 1] = std::move(blob); + _cur_off = 0; + ++_cur_blob; } Metrics::IdType @@ -113,15 +114,17 @@ Metrics::Storage::lookup(const std::string_view name) const Metrics::AtomicType * Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics::MetricType *out_type) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything that does not name an allocated slot resolves to the reserved bad_id slot rather than + // indexing out of range. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + if (out_name) { *out_name = std::get<0>(std::get<0>(*blob)[offset]); } @@ -159,15 +162,17 @@ Metrics::Storage::lookup(const std::string_view name, Metrics::IdType *out_id, M std::string_view Metrics::Storage::name(Metrics::IdType id) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything that does not name an allocated slot resolves to the reserved bad_id slot rather than + // indexing out of range. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + const std::string &result = std::get<0>(std::get<0>(*blob)[offset]); return result; @@ -225,14 +230,14 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT bool Metrics::Storage::rename(Metrics::IdType id, std::string_view name) { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); - // We can only rename Metrics that are already allocated - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { + if (!_is_allocated(id)) { return false; } + auto [blob_ix, offset] = _splitID(id); + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); std::lock_guard lock(_mutex); diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 960324997b0..31ffc3f4f84 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -640,3 +640,27 @@ TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]" REQUIRE(Metrics::Counter::load(p) == 7); REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p); } + +TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") +{ + // The offset field of an id is 16 bits, but a real offset is always below MAX_SIZE. Ids reaching + // the id based accessors come from plugins via TSStat*, so a malformed offset must not index past + // the end of a blob's 1024 entry arrays. Force a second blob first: with only one blob every + // blob_ix != 0 is unallocated and would be caught by the null check alone. + auto &h = Metrics::hidden_instance(); + + for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr("f1.fill." + std::to_string(i)) != nullptr); + } + + auto const *bad = h.lookup(Metrics::IdType{0}); // the reserved bad_id slot + REQUIRE(bad != nullptr); + + // blob 0 is allocated, so the null check does not fire; only the MAX_SIZE test stands between + // this and atomics[65535]. + for (Metrics::IdType id : {Metrics::IdType{0x0000FFFF}, Metrics::IdType{0x00000400}, Metrics::IdType{0x0001FFFF}}) { + REQUIRE(h.valid(id) == false); + REQUIRE(h.lookup(id) == bad); + REQUIRE(h.name(id) == h.name(Metrics::IdType{0})); + } +} From a2a64337386237a933087724a7c64751f719b9c3 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 19 Aug 2026 10:03:12 -0500 Subject: [PATCH 02/13] Metrics: publish the allocation point with release/acquire The lock removal in #13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race #13310 took the mutex to close. Close it without the mutex instead. Making each counter atomic does not make the pair update atomically, and it does not need to. _cur_blob and _cur_off are publication points: each is written last, with a release store, after whatever it makes visible -- the blob pointer and the reset offset for _cur_blob, the slot's name for _cur_off. A reader acquires _cur_blob first, so observing a value for it also observes everything addBlob() wrote before releasing it. The torn pair a reader could otherwise see, a new blob index with the previous blob's stale offset, is unreachable rather than merely unlikely, so neither a packed word nor per-blob counters are needed. _blobs stays non-atomic. It is only read at an index no greater than _cur_blob, and that write is sequenced before the release store the reader acquired, so there is no race to close. Writers all hold the mutex and load relaxed. What remains is that a reader can observe an older _cur_blob with an already reset _cur_off and reject an id naming the previous blob, which drops an increment rather than misattributing one. Verified with a TSAN harness running eight readers validating and resolving ids across the whole space while a writer creates 2600 metrics across several blob boundaries: three reported races before this change, none after. --- include/tsutil/Metrics.h | 35 ++++++++++++++++++-------- src/tsutil/Metrics.cc | 54 ++++++++++++++++++++++++++-------------- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index ffdc97768ab..8ff810ad01e 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -320,11 +320,20 @@ class Metrics class Storage { - BlobStorage _blobs; - uint16_t _cur_blob = 0; - uint16_t _cur_off = 0; - LookupTable _lookups; - mutable std::mutex _mutex; + /* _cur_blob and _cur_off are the two publication points. Each is written last, with a release + * store, after whatever it makes visible: the blob pointer and the reset of _cur_off for + * _cur_blob, the slot's name for _cur_off. A reader loads them with acquire, _cur_blob first -- + * see _is_allocated(). That is what makes the pair consistent without reading it as one word, + * and it is why _blobs itself needs no atomic: it is only ever read at an index no greater than + * _cur_blob, and that write is sequenced before the release store the reader acquired. + * + * Writers all hold _mutex, so they load these relaxed; there is no other writer to race with. + */ + BlobStorage _blobs; + std::atomic _cur_blob{0}; + std::atomic _cur_off{0}; + LookupTable _lookups; + mutable std::mutex _mutex; public: Storage(const Storage &) = delete; @@ -354,7 +363,7 @@ class Metrics current() const { std::lock_guard lock(_mutex); - return {_cur_blob, _cur_off}; + return {_cur_blob.load(std::memory_order_relaxed), _cur_off.load(std::memory_order_relaxed)}; } /** Whether @a id names a slot that has actually been allocated. @@ -375,12 +384,16 @@ class Metrics auto [blob_ix, offset] = _splitID(id); + // Load the outer publication point first: seeing a value for _cur_blob means everything + // addBlob() wrote before releasing it -- the blob pointer, and the reset of _cur_off -- is + // visible here too. Reading _cur_off first would defeat that. + auto const cur_blob = _cur_blob.load(std::memory_order_acquire); + auto const cur_off = _cur_off.load(std::memory_order_acquire); + // The blob comparison is against <= / <, not a test for "not the current blob", because - // addBlob() stores a new blob before advancing _cur_blob: for those two instructions - // _blobs[_cur_blob + 1] is non-null while still holding nothing. Requiring the index to be no - // greater than _cur_blob, and the offset to be below _cur_off in that blob, is correct - // whichever of the two the reader happens to observe first. - return offset < MAX_SIZE && blob_ix <= _cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < _cur_blob || offset < _cur_off); + // addBlob() stores a new blob before advancing _cur_blob: until it does, _blobs[cur_blob + 1] + // is non-null while still holding nothing. + return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < cur_blob || offset < cur_off); } bool diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 5d7b7072ace..2957167efb1 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,13 +58,18 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); + auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); + debug_assert(blob); - // The write below is to _blobs[_cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. - release_assert(_cur_blob < MAX_BLOBS - 1); + // The write below is to _blobs[cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. + release_assert(cur_blob < MAX_BLOBS - 1); + + _blobs[cur_blob + 1] = std::move(blob); + _cur_off.store(0, std::memory_order_relaxed); - _blobs[_cur_blob + 1] = std::move(blob); - _cur_off = 0; - ++_cur_blob; + // Publishes the new blob. Both writes above are sequenced before this, so a reader that acquires + // this value sees the blob pointer and the reset offset. + _cur_blob.store(cur_blob + 1, std::memory_order_release); } Metrics::IdType @@ -80,18 +85,25 @@ Metrics::Storage::create(std::string_view name, const MetricType type) // The slot is written below and the bookkeeping only then advances, calling addBlob() once // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) { + auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); + auto const cur_off = _cur_off.load(std::memory_order_relaxed); + + if (cur_blob >= MAX_BLOBS - 1 && cur_off >= MAX_SIZE - 1) { return 0; // Slot 0 is the reserved bad_id. Cannot grow further. } - Metrics::IdType id = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); + Metrics::IdType id = _makeId(cur_blob, cur_off, type); + Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); Metrics::NameStorage &names = std::get<0>(*blob); - names[_cur_off] = std::make_tuple(std::string(name), id); - _lookups.emplace(std::get<0>(names[_cur_off]), id); + names[cur_off] = std::make_tuple(std::string(name), id); + _lookups.emplace(std::get<0>(names[cur_off]), id); - if (++_cur_off >= MAX_SIZE) { + // Publishes the slot. The name write above is sequenced before this, so a reader that acquires + // this value can read the name. + _cur_off.store(cur_off + 1, std::memory_order_release); + + if (cur_off + 1 >= MAX_SIZE) { addBlob(); // This resets _cur_off to 0 as well } @@ -193,7 +205,7 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same // cost: some slots of the last blob go unused. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) { + if (_cur_blob.load(std::memory_order_relaxed) >= MAX_BLOBS - 1 && _cur_off.load(std::memory_order_relaxed) + size >= MAX_SIZE) { if (id) { *id = 0; // Slot 0 is the reserved bad_id. } @@ -201,26 +213,32 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT } // A span has to be contiguous, so one that does not fit in the current blob starts a new one. - if (_cur_off + size > MAX_SIZE) { + if (_cur_off.load(std::memory_order_relaxed) + size > MAX_SIZE) { addBlob(); } - Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); + // Re-read: addBlob() above may have moved both. + auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); + auto const cur_off = _cur_off.load(std::memory_order_relaxed); + + Metrics::IdType span_start = _makeId(cur_blob, cur_off, type); + Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); Metrics::AtomicStorage &atomics = std::get<1>(*blob); - Metrics::SpanType span = Metrics::SpanType(&atomics[_cur_off], size); + Metrics::SpanType span = Metrics::SpanType(&atomics[cur_off], size); if (id) { *id = span_start; } - _cur_off += size; + // Publishes the span's slots. Unlike create() there are no names to make visible, but a reader + // still must not see the offset advance before the blob it advanced within. + _cur_off.store(cur_off + size, std::memory_order_release); // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next create() writes one past the // end of the blob's name array. It also makes end() unreachable for iterator::next(), which // wraps on ++offset == MAX_SIZE. - if (_cur_off >= MAX_SIZE) { + if (cur_off + size >= MAX_SIZE) { addBlob(); } From e3e48f0277ffad7ae9167f09ba4acb13d3b79482 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Fri, 21 Aug 2026 14:53:32 -0500 Subject: [PATCH 03/13] Metrics: cover concurrent id lookup, and make _extractType total Add a test that resolves ids from several threads while another registers metrics across a few blob boundaries. Nothing single threaded exercises the publication order the previous commit relies on; under the tsan preset, making either allocation counter non-atomic again reports a data race here. The test cannot catch a downgrade of the release/acquire pairs to relaxed -- atomics are race free at any ordering -- and says so, so the memory orders are not mistaken for tested. _extractType shifted a signed IdType, so _extractType(NOT_FOUND) sign extended to -4, a MetricType outside its enumeration, returned by Metrics::type(). Shifting unsigned is not enough on its own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Mask to the single bit _makeId writes, which makes the function total for any input. --- include/tsutil/Metrics.h | 2 +- src/tsutil/unit_tests/test_Metrics.cc | 69 +++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 8ff810ad01e..dfb79d8fddf 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -308,7 +308,7 @@ class Metrics static constexpr MetricType _extractType(IdType value) { - return MetricType{value >> METRIC_TYPE_BITS}; + return MetricType{static_cast((static_cast(value) >> METRIC_TYPE_BITS) & 0x1)}; } static constexpr IdType diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 31ffc3f4f84..c04805dbac3 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -664,3 +665,71 @@ TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics] REQUIRE(h.name(id) == h.name(Metrics::IdType{0})); } } + +TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][Metrics]") +{ + // Storage has no lock on the id based read paths, so a reader resolving an id races a writer + // registering a new metric -- a plugin TSStatCreate or a config reload against live traffic. + // Safety rests on _cur_blob and _cur_off being atomic, release stored after whatever they + // publish, and acquire loaded with _cur_blob first. Driving both sides at once is what makes a + // regression visible: under the tsan preset, making either counter non-atomic again reports a + // data race here. + // + // What this does NOT catch is a downgrade of those stores and loads to relaxed. Atomics are + // race free at any ordering, so TSAN stays quiet and the assertions below still hold; only a + // weakly ordered machine would ever observe the difference, and not reliably. Treat the memory + // orders in Storage as reviewed rather than tested. + // + // Enough metrics to cross several blob boundaries, which is where the publication order matters. + constexpr int N_READERS = 4; + constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; + auto &h = Metrics::hidden_instance(); + std::atomic stop{false}; + std::atomic mismatches{0}; + + std::vector readers; + + for (int t = 0; t < N_READERS; ++t) { + readers.emplace_back([&]() { + while (!stop.load(std::memory_order_relaxed)) { + for (Metrics::IdType id = 0; id < N_CREATE; ++id) { + if (!h.valid(id)) { + continue; + } + + // valid() said this id names an allocated slot, so lookup() must agree and hand back a + // real metric rather than clamping to the reserved bad_id slot. A publication ordering + // mistake shows up here as a name that is still empty. + std::string_view name; + Metrics::MetricType type; + auto *m = h.lookup(id, &name, &type); + + if (m == nullptr || (id != 0 && name.empty())) { + mismatches.fetch_add(1, std::memory_order_relaxed); + } + } + } + }); + } + + for (int i = 0; i < N_CREATE; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr("pub.order." + std::to_string(i)) != nullptr); + } + + stop.store(true, std::memory_order_relaxed); + for (auto &r : readers) { + r.join(); + } + + CHECK(mismatches.load() == 0); + + // Everything the writer created must be resolvable by name and by id afterwards. + for (int i = 0; i < N_CREATE; ++i) { + auto const nm = "pub.order." + std::to_string(i); + auto const id = h.lookup(nm); + + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(h.valid(id)); + REQUIRE(h.name(id) == nm); + } +} From f2bbd582e89e4b6ee8e869b681a52f31ceccad18 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Fri, 21 Aug 2026 15:39:05 -0500 Subject: [PATCH 04/13] Add a ts::Metrics micro benchmark Nothing in tree measured the metric read paths, which is why a global mutex on the hottest one went unnoticed until it showed up in a production profile. Four cases, scaled by thread count: increment(id) what TSStatIntIncrement does, the path that regressed increment(ptr) what core and cripts do, the floor lookup(id) the lock free id resolution alone lookup(name) the same resolution through the mutex guarded name map lookup(name) is deliberately included as a positive control. It still takes the lock, so it must degrade with thread count; if it ever stops doing so, the harness is not loading the machine and the other three numbers mean nothing. Built only with ENABLE_BENCHMARKS, as with the rest of tools/benchmark. --- tools/benchmark/CMakeLists.txt | 3 + tools/benchmark/benchmark_Metrics.cc | 199 +++++++++++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 tools/benchmark/benchmark_Metrics.cc diff --git a/tools/benchmark/CMakeLists.txt b/tools/benchmark/CMakeLists.txt index e58e8dcf26b..ba3816cf6be 100644 --- a/tools/benchmark/CMakeLists.txt +++ b/tools/benchmark/CMakeLists.txt @@ -33,6 +33,9 @@ target_link_libraries(benchmark_ProxyAllocator PRIVATE Catch2::Catch2WithMain ts add_executable(benchmark_SharedMutex benchmark_SharedMutex.cc) target_link_libraries(benchmark_SharedMutex PRIVATE Catch2::Catch2 ts::tscore libswoc::libswoc) +add_executable(benchmark_Metrics benchmark_Metrics.cc) +target_link_libraries(benchmark_Metrics PRIVATE Catch2::Catch2 ts::tsutil libswoc::libswoc) + add_executable(benchmark_Random benchmark_Random.cc) target_link_libraries(benchmark_Random PRIVATE Catch2::Catch2WithMain ts::tscore) diff --git a/tools/benchmark/benchmark_Metrics.cc b/tools/benchmark/benchmark_Metrics.cc new file mode 100644 index 00000000000..095c2ea221c --- /dev/null +++ b/tools/benchmark/benchmark_Metrics.cc @@ -0,0 +1,199 @@ +/** @file + + Micro benchmark tool for ts::Metrics + + The metric values are lock free atomics, but reaching one from an id is not free, and the read + paths that get there have very different costs. Four cases, all scaled by thread count because + contention is the interesting axis: + + increment(id) what TSStatIntIncrement does: valid() then lookup(id) then fetch_add + increment(ptr) what core, cripts and a few plugins do: a bare fetch_add on a cached pointer + lookup(id) the lock free id resolution on its own + lookup(name) the same resolution by name, which still takes Storage's mutex + + increment(id) against increment(ptr) is the cost a plugin pays for having only an id. lookup(id) + against lookup(name) isolates the mutex: the two do comparable work, so a gap that widens with + thread count is contention rather than instructions. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you 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. + */ + +#define CATCH_CONFIG_ENABLE_BENCHMARKING + +#include +#include +#include + +#include "tsutil/Metrics.h" + +#include +#include +#include +#include + +using ts::Metrics; + +namespace +{ +// Args +struct Conf { + int nthreads = 1; + int nops = 1000; + int nmetrics = 64; +}; + +Conf conf; + +/// The metrics every case operates on, created once so no case pays for creation. +struct Fixture { + std::vector ids; + std::vector ptrs; + std::vector names; + + Fixture() + { + auto &m = Metrics::instance(); + + ids.reserve(conf.nmetrics); + ptrs.reserve(conf.nmetrics); + names.reserve(conf.nmetrics); + + for (int i = 0; i < conf.nmetrics; ++i) { + names.push_back("benchmark.metrics." + std::to_string(i)); + + // createPtr is what core and cripts do: create once at init and keep the pointer. It also + // registers the name, so the id and name lookups below resolve to the same metric. + ptrs.push_back(Metrics::Counter::createPtr(names.back())); + ids.push_back(m.lookup(names.back())); + } + } +}; + +Fixture *fixture = nullptr; + +/** Run @a op on every thread, @c nops times each, and return a value derived from the results. + * + * The return value exists so nothing can be optimized away; it is not meaningful. + */ +template +int64_t +run(F &&op) +{ + std::vector threads; + std::atomic sink{0}; + + threads.reserve(conf.nthreads); + + for (int t = 0; t < conf.nthreads; ++t) { + threads.emplace_back([t, &sink, &op]() { + int64_t local = 0; + + for (int i = 0; i < conf.nops; ++i) { + // Stride the starting point per thread so they are not all hammering one metric, which + // would measure cacheline ping-pong on that one atomic rather than the lookup path. + local += op((t + i) % conf.nmetrics); + } + sink.fetch_add(local, std::memory_order_relaxed); + }); + } + + for (auto &th : threads) { + th.join(); + } + + return sink.load(); +} + +} // namespace + +TEST_CASE("Micro benchmark of ts::Metrics", "") +{ + auto &m = Metrics::instance(); + + SECTION("increment by id") + { + // The TSStatIntIncrement path: validation, then id resolution, then the add. + BENCHMARK("increment(id)") + { + return run([&m](int i) -> int64_t { + auto id = fixture->ids[i]; + + return m.valid(id) ? m.increment(id, 1) : 0; + }); + }; + } + + SECTION("increment by cached pointer") + { + // What core does. This is the floor: no resolution at all. + BENCHMARK("increment(ptr)") + { + return run([](int i) -> int64_t { + Metrics::Counter::increment(fixture->ptrs[i], 1); + + return 1; + }); + }; + } + + SECTION("lookup by id") + { + // Lock free resolution. + BENCHMARK("lookup(id)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->ids[i]) != nullptr; }); + }; + } + + SECTION("lookup by name") + { + // The same resolution, but through the mutex guarded name map. + BENCHMARK("lookup(name)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->names[i]) != Metrics::NOT_FOUND; }); + }; + } +} + +int +main(int argc, char *argv[]) +{ + Catch::Session session; + + using namespace Catch::Clara; + + // clang-format off + auto cli = session.cli() | + Opt(conf.nthreads, "")["--ts-nthreads"]("number of threads (default: 1)") | + Opt(conf.nops, "")["--ts-nops"]("operations per thread per run (default: 1000)") | + Opt(conf.nmetrics, "")["--ts-nmetrics"]("distinct metrics to spread across (default: 64)"); + // clang-format on + + session.cli(cli); + + int returnCode = session.applyCommandLine(argc, argv); + if (returnCode != 0) { + return returnCode; + } + + Fixture f; + fixture = &f; + + return session.run(); +} From d10247ba0b851faddda7622b6dd54e05b782121b Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Fri, 21 Aug 2026 15:57:06 -0500 Subject: [PATCH 05/13] Metrics: trim comments to the invariants State what holds rather than how it came to hold. Drops the explanations of which write order a comparison compensates for, what a reader would have seen otherwise, and what each benchmark case is meant to prove. Also shortens the createSpan boundary test's preamble, which describes the bug it covers at more length than the assertion needs. --- include/tsutil/Metrics.h | 32 ++++++++--------------- src/tsutil/Metrics.cc | 15 ++++------- src/tsutil/unit_tests/test_Metrics.cc | 35 ++++++++----------------- tools/benchmark/benchmark_Metrics.cc | 37 ++++++++++----------------- 4 files changed, 40 insertions(+), 79 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index dfb79d8fddf..68665dd2de7 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -320,14 +320,10 @@ class Metrics class Storage { - /* _cur_blob and _cur_off are the two publication points. Each is written last, with a release - * store, after whatever it makes visible: the blob pointer and the reset of _cur_off for - * _cur_blob, the slot's name for _cur_off. A reader loads them with acquire, _cur_blob first -- - * see _is_allocated(). That is what makes the pair consistent without reading it as one word, - * and it is why _blobs itself needs no atomic: it is only ever read at an index no greater than - * _cur_blob, and that write is sequenced before the release store the reader acquired. - * - * Writers all hold _mutex, so they load these relaxed; there is no other writer to race with. + /* _cur_blob and _cur_off are release stored last, after whatever they publish: the blob pointer + * for _cur_blob, the slot's name for _cur_off. Readers acquire load them, _cur_blob first. + * _blobs needs no atomic because it is only read at an index no greater than _cur_blob. + * Writers hold _mutex and load relaxed. */ BlobStorage _blobs; std::atomic _cur_blob{0}; @@ -366,14 +362,11 @@ class Metrics return {_cur_blob.load(std::memory_order_relaxed), _cur_off.load(std::memory_order_relaxed)}; } - /** Whether @a id names a slot that has actually been allocated. + /** Whether @a id names an allocated slot. * - * The single gate for every id based accessor. Ids arriving through the @c TSStat* API are - * plugin supplied and untrusted, so three things have to hold: the id is not negative, the - * offset is one @c _makeId could have produced -- the offset field is 16 bits wide but a real - * offset is always below @c MAX_SIZE, so a larger one is malformed rather than merely stale -- - * and the slot has been handed out. Blobs are filled in increasing order and never freed, so - * that last part means either an earlier blob, or below the allocation point in the current one. + * The gate for every id based accessor, since ids from the @c TSStat* API are untrusted. An id + * qualifies when it is non-negative, its offset is one @c _makeId could produce, and its slot + * has been handed out. */ bool _is_allocated(IdType id) const @@ -384,15 +377,12 @@ class Metrics auto [blob_ix, offset] = _splitID(id); - // Load the outer publication point first: seeing a value for _cur_blob means everything - // addBlob() wrote before releasing it -- the blob pointer, and the reset of _cur_off -- is - // visible here too. Reading _cur_off first would defeat that. + // _cur_blob first: acquiring it also makes visible everything published under it. auto const cur_blob = _cur_blob.load(std::memory_order_acquire); auto const cur_off = _cur_off.load(std::memory_order_acquire); - // The blob comparison is against <= / <, not a test for "not the current blob", because - // addBlob() stores a new blob before advancing _cur_blob: until it does, _blobs[cur_blob + 1] - // is non-null while still holding nothing. + // A non-null blob past cur_blob is allocated but not yet published, hence <= and < rather + // than a test for "not the current blob". return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < cur_blob || offset < cur_off); } diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 2957167efb1..0c45722b4d0 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -67,8 +67,7 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! _blobs[cur_blob + 1] = std::move(blob); _cur_off.store(0, std::memory_order_relaxed); - // Publishes the new blob. Both writes above are sequenced before this, so a reader that acquires - // this value sees the blob pointer and the reset offset. + // Publishes the blob; both writes above are sequenced before it. _cur_blob.store(cur_blob + 1, std::memory_order_release); } @@ -99,8 +98,7 @@ Metrics::Storage::create(std::string_view name, const MetricType type) names[cur_off] = std::make_tuple(std::string(name), id); _lookups.emplace(std::get<0>(names[cur_off]), id); - // Publishes the slot. The name write above is sequenced before this, so a reader that acquires - // this value can read the name. + // Publishes the slot; the name write above is sequenced before it. _cur_off.store(cur_off + 1, std::memory_order_release); if (cur_off + 1 >= MAX_SIZE) { @@ -128,8 +126,7 @@ Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics { auto [blob_ix, offset] = _splitID(id); - // Anything that does not name an allocated slot resolves to the reserved bad_id slot rather than - // indexing out of range. + // Anything not naming an allocated slot resolves to the reserved bad_id slot. if (!_is_allocated(id)) { blob_ix = 0; offset = 0; @@ -176,8 +173,7 @@ Metrics::Storage::name(Metrics::IdType id) const { auto [blob_ix, offset] = _splitID(id); - // Anything that does not name an allocated slot resolves to the reserved bad_id slot rather than - // indexing out of range. + // Anything not naming an allocated slot resolves to the reserved bad_id slot. if (!_is_allocated(id)) { blob_ix = 0; offset = 0; @@ -230,8 +226,7 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT *id = span_start; } - // Publishes the span's slots. Unlike create() there are no names to make visible, but a reader - // still must not see the offset advance before the blob it advanced within. + // Publishes the span's slots. _cur_off.store(cur_off + size, std::memory_order_release); // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index c04805dbac3..5d2ec771f9e 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -611,14 +611,9 @@ TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]") { - // A span has to be contiguous, so createSpan(MAX_SIZE) always starts a fresh blob and then fills - // it completely, whatever the current offset was. That makes this the one span size that reaches - // the boundary case deterministically: the offset ends up at MAX_SIZE, and unlike create(), - // createSpan used not to grow a new blob afterwards. The next create() then indexed one past the - // end of the blob's name array, and end() became an id that iterator::next() can never reach - // because it wraps at ++offset == MAX_SIZE. - // - // createSpan only ever targets the published store, so this necessarily allocates there. + // A span of MAX_SIZE always lands at offset 0 of an empty blob and fills it, whatever the current + // offset was, so it reaches the blob boundary deterministically. createSpan only targets the + // published store, so this allocates there. Metrics::IdType span_id = Metrics::NOT_FOUND; auto span = Metrics::Counter::createSpan(Metrics::MAX_SIZE, &span_id); @@ -644,10 +639,9 @@ TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]" TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") { - // The offset field of an id is 16 bits, but a real offset is always below MAX_SIZE. Ids reaching - // the id based accessors come from plugins via TSStat*, so a malformed offset must not index past - // the end of a blob's 1024 entry arrays. Force a second blob first: with only one blob every - // blob_ix != 0 is unallocated and would be caught by the null check alone. + // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a malformed one must + // not index past a blob's arrays. Two blobs are needed for the offset check to be what rejects + // it; with one, the null blob check would. auto &h = Metrics::hidden_instance(); for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) { @@ -668,19 +662,10 @@ TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics] TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][Metrics]") { - // Storage has no lock on the id based read paths, so a reader resolving an id races a writer - // registering a new metric -- a plugin TSStatCreate or a config reload against live traffic. - // Safety rests on _cur_blob and _cur_off being atomic, release stored after whatever they - // publish, and acquire loaded with _cur_blob first. Driving both sides at once is what makes a - // regression visible: under the tsan preset, making either counter non-atomic again reports a - // data race here. - // - // What this does NOT catch is a downgrade of those stores and loads to relaxed. Atomics are - // race free at any ordering, so TSAN stays quiet and the assertions below still hold; only a - // weakly ordered machine would ever observe the difference, and not reliably. Treat the memory - // orders in Storage as reviewed rather than tested. - // - // Enough metrics to cross several blob boundaries, which is where the publication order matters. + // The id based read paths take no lock, so resolving an id races a concurrent create. Run both + // sides at once, across enough metrics to cross several blob boundaries. Under the tsan preset a + // non-atomic allocation counter reports a data race here; relaxing the memory orders does not, + // since atomics are race free at any ordering. constexpr int N_READERS = 4; constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; auto &h = Metrics::hidden_instance(); diff --git a/tools/benchmark/benchmark_Metrics.cc b/tools/benchmark/benchmark_Metrics.cc index 095c2ea221c..e42b092a7f4 100644 --- a/tools/benchmark/benchmark_Metrics.cc +++ b/tools/benchmark/benchmark_Metrics.cc @@ -2,18 +2,17 @@ Micro benchmark tool for ts::Metrics - The metric values are lock free atomics, but reaching one from an id is not free, and the read - paths that get there have very different costs. Four cases, all scaled by thread count because - contention is the interesting axis: + Metric values are lock free atomics; reaching one from an id is not. Four cases, scaled by thread + count: - increment(id) what TSStatIntIncrement does: valid() then lookup(id) then fetch_add - increment(ptr) what core, cripts and a few plugins do: a bare fetch_add on a cached pointer - lookup(id) the lock free id resolution on its own - lookup(name) the same resolution by name, which still takes Storage's mutex + increment(id) valid() then lookup(id) then fetch_add, as TSStatIntIncrement does + increment(ptr) a bare fetch_add on a cached pointer, as core does + lookup(id) lock free id resolution + lookup(name) the same resolution through the mutex guarded name map - increment(id) against increment(ptr) is the cost a plugin pays for having only an id. lookup(id) - against lookup(name) isolates the mutex: the two do comparable work, so a gap that widens with - thread count is contention rather than instructions. + increment(id) against increment(ptr) is what an id costs a plugin. lookup(id) against + lookup(name) isolates the mutex, and serves as a control: it must degrade with thread count, or + the harness is not loading the machine. @section license License @@ -60,7 +59,7 @@ struct Conf { Conf conf; -/// The metrics every case operates on, created once so no case pays for creation. +/// The metrics every case operates on, created once. struct Fixture { std::vector ids; std::vector ptrs; @@ -77,8 +76,7 @@ struct Fixture { for (int i = 0; i < conf.nmetrics; ++i) { names.push_back("benchmark.metrics." + std::to_string(i)); - // createPtr is what core and cripts do: create once at init and keep the pointer. It also - // registers the name, so the id and name lookups below resolve to the same metric. + // Registers the name too, so the id and name lookups resolve to the same metric. ptrs.push_back(Metrics::Counter::createPtr(names.back())); ids.push_back(m.lookup(names.back())); } @@ -87,10 +85,7 @@ struct Fixture { Fixture *fixture = nullptr; -/** Run @a op on every thread, @c nops times each, and return a value derived from the results. - * - * The return value exists so nothing can be optimized away; it is not meaningful. - */ +/// Run @a op on every thread, @c nops times each. The return value only defeats optimization. template int64_t run(F &&op) @@ -105,8 +100,7 @@ run(F &&op) int64_t local = 0; for (int i = 0; i < conf.nops; ++i) { - // Stride the starting point per thread so they are not all hammering one metric, which - // would measure cacheline ping-pong on that one atomic rather than the lookup path. + // Stride per thread, or this measures cacheline ping-pong on one atomic. local += op((t + i) % conf.nmetrics); } sink.fetch_add(local, std::memory_order_relaxed); @@ -128,7 +122,6 @@ TEST_CASE("Micro benchmark of ts::Metrics", "") SECTION("increment by id") { - // The TSStatIntIncrement path: validation, then id resolution, then the add. BENCHMARK("increment(id)") { return run([&m](int i) -> int64_t { @@ -141,7 +134,7 @@ TEST_CASE("Micro benchmark of ts::Metrics", "") SECTION("increment by cached pointer") { - // What core does. This is the floor: no resolution at all. + // The floor: no resolution at all. BENCHMARK("increment(ptr)") { return run([](int i) -> int64_t { @@ -154,7 +147,6 @@ TEST_CASE("Micro benchmark of ts::Metrics", "") SECTION("lookup by id") { - // Lock free resolution. BENCHMARK("lookup(id)") { return run([&m](int i) -> int64_t { return m.lookup(fixture->ids[i]) != nullptr; }); @@ -163,7 +155,6 @@ TEST_CASE("Micro benchmark of ts::Metrics", "") SECTION("lookup by name") { - // The same resolution, but through the mutex guarded name map. BENCHMARK("lookup(name)") { return run([&m](int i) -> int64_t { return m.lookup(fixture->names[i]) != Metrics::NOT_FOUND; }); From ddced0b6bb6631bd72123570725ddedc8646926c Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 24 Aug 2026 11:32:04 -0500 Subject: [PATCH 06/13] Metrics: probe real ids in the concurrent lookup test The reader swept ids as consecutive integers, but an id packs the blob index above the offset, so 0..N only ever named blob 0 and everything from MAX_SIZE up decoded to an offset that validation rejects. Earlier cases in this file leave blob 0 full, so the reader was walking settled slots while the writer worked in a blob it never named. Take ids from what the writer has registered instead, and assert the ids span more than one blob so a future change cannot quietly confine the sweep again. Also assert the readers resolved something, since every id being skipped would otherwise pass. --- src/tsutil/unit_tests/test_Metrics.cc | 56 +++++++++++++++++++++------ 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 5d2ec771f9e..25dd78c8a49 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -663,42 +664,63 @@ TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics] TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][Metrics]") { // The id based read paths take no lock, so resolving an id races a concurrent create. Run both - // sides at once, across enough metrics to cross several blob boundaries. Under the tsan preset a + // sides at once, over enough metrics to cross several blob boundaries. Under the tsan preset a // non-atomic allocation counter reports a data race here; relaxing the memory orders does not, // since atomics are race free at any ordering. - constexpr int N_READERS = 4; - constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; - auto &h = Metrics::hidden_instance(); + // + // Readers take ids from what the writer has registered rather than counting integers: an id packs + // the blob index above the offset, so consecutive integers only ever name the first blob. The + // store is relaxed, so a reader can pick up an id whose slot is not published yet, which is the + // case of interest. + constexpr int N_READERS = 4; + constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; + + auto &h = Metrics::hidden_instance(); std::atomic stop{false}; std::atomic mismatches{0}; + std::atomic resolved{0}; + + std::vector> created(N_CREATE); + + for (auto &c : created) { + c.store(Metrics::NOT_FOUND, std::memory_order_relaxed); + } std::vector readers; for (int t = 0; t < N_READERS; ++t) { readers.emplace_back([&]() { + int n = 0; + while (!stop.load(std::memory_order_relaxed)) { - for (Metrics::IdType id = 0; id < N_CREATE; ++id) { - if (!h.valid(id)) { + for (int i = 0; i < N_CREATE; ++i) { + auto const id = created[i].load(std::memory_order_relaxed); + + if (id == Metrics::NOT_FOUND || !h.valid(id)) { continue; } - // valid() said this id names an allocated slot, so lookup() must agree and hand back a - // real metric rather than clamping to the reserved bad_id slot. A publication ordering - // mistake shows up here as a name that is still empty. + // valid() accepted the id, so lookup() must hand back the metric with its name rather + // than clamping to the reserved bad_id slot. std::string_view name; Metrics::MetricType type; auto *m = h.lookup(id, &name, &type); - if (m == nullptr || (id != 0 && name.empty())) { + if (m == nullptr || name.empty()) { mismatches.fetch_add(1, std::memory_order_relaxed); } + ++n; } } + resolved.fetch_add(n, std::memory_order_relaxed); }); } for (int i = 0; i < N_CREATE; ++i) { - REQUIRE(Metrics::Counter::createHiddenPtr("pub.order." + std::to_string(i)) != nullptr); + auto const nm = "pub.order." + std::to_string(i); + + REQUIRE(Metrics::Counter::createHiddenPtr(nm) != nullptr); + created[i].store(h.lookup(nm), std::memory_order_relaxed); } stop.store(true, std::memory_order_relaxed); @@ -707,8 +729,11 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M } CHECK(mismatches.load() == 0); + CHECK(resolved.load() > 0); + + auto lo = std::numeric_limits::max(); + auto hi = std::numeric_limits::min(); - // Everything the writer created must be resolvable by name and by id afterwards. for (int i = 0; i < N_CREATE; ++i) { auto const nm = "pub.order." + std::to_string(i); auto const id = h.lookup(nm); @@ -716,5 +741,12 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M REQUIRE(id != Metrics::NOT_FOUND); REQUIRE(h.valid(id)); REQUIRE(h.name(id) == nm); + + lo = std::min(lo, id); + hi = std::max(hi, id); } + + // More metrics than fit in one blob, so the ids must span blobs. A spread no wider than a blob + // would mean the sweep above never left the first one. + REQUIRE(hi - lo > Metrics::MAX_SIZE); } From bc333f401eb84dc25c4e6750889e39d8dee18273 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 24 Aug 2026 13:20:29 -0500 Subject: [PATCH 07/13] Metrics: make _is_allocated private, tidy createSpan's index handling _is_allocated is only called by Storage's own accessors, so it does not belong in the public section; valid() remains the public gate. createSpan loaded _cur_off twice and _cur_blob once for its two guards, then re-read both unconditionally in case addBlob() had moved them. Load the pair once and refresh it only in the branch that grows a blob, which drops two atomic loads from the common path. Re-reading rather than adjusting the locals by hand keeps the caller from restating what addBlob() sets. --- include/tsutil/Metrics.h | 13 +++++++------ src/tsutil/Metrics.cc | 13 +++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 68665dd2de7..f5cd7763eb7 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -362,6 +362,13 @@ class Metrics return {_cur_blob.load(std::memory_order_relaxed), _cur_off.load(std::memory_order_relaxed)}; } + bool + valid(IdType id) const + { + return _is_allocated(id); + } + + private: /** Whether @a id names an allocated slot. * * The gate for every id based accessor, since ids from the @c TSStat* API are untrusted. An id @@ -385,12 +392,6 @@ class Metrics // than a test for "not the current blob". return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < cur_blob || offset < cur_off); } - - bool - valid(IdType id) const - { - return _is_allocated(id); - } }; Metrics(std::shared_ptr &str) : _storage(str) {} diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 0c45722b4d0..74ab2be9c54 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -201,7 +201,10 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same // cost: some slots of the last blob go unused. - if (_cur_blob.load(std::memory_order_relaxed) >= MAX_BLOBS - 1 && _cur_off.load(std::memory_order_relaxed) + size >= MAX_SIZE) { + auto cur_blob = _cur_blob.load(std::memory_order_relaxed); + auto cur_off = _cur_off.load(std::memory_order_relaxed); + + if (cur_blob >= MAX_BLOBS - 1 && cur_off + size >= MAX_SIZE) { if (id) { *id = 0; // Slot 0 is the reserved bad_id. } @@ -209,14 +212,12 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT } // A span has to be contiguous, so one that does not fit in the current blob starts a new one. - if (_cur_off.load(std::memory_order_relaxed) + size > MAX_SIZE) { + if (cur_off + size > MAX_SIZE) { addBlob(); + cur_blob = _cur_blob.load(std::memory_order_relaxed); + cur_off = _cur_off.load(std::memory_order_relaxed); } - // Re-read: addBlob() above may have moved both. - auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); - auto const cur_off = _cur_off.load(std::memory_order_relaxed); - Metrics::IdType span_start = _makeId(cur_blob, cur_off, type); Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); Metrics::AtomicStorage &atomics = std::get<1>(*blob); From 96e8e5b42ea67feb02868bee6327e0b7c804d2b0 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 2 Sep 2026 17:22:19 -0500 Subject: [PATCH 08/13] Publish the next free slot as one packed value _cur_blob and _cur_off were two atomics, and readers need the pair to be coherent. addBlob() reset the offset then bumped the blob, so a reader between the two saw the old blob with a zero offset and rejected every id in the just-completed blob. With ENABLE_FAST_SDK=OFF that reaches _TSReleaseAssert through TSStatInt*, so it aborts rather than losing a count. Reversing the stores only trades it for accepting ids in a blob nothing has been written to; two atomics have no coherent pair either way. One atomic holding the blob index above the offset, packed as an id is packed, fixes it: crossing a blob is a single release store. The value only ever increases, so an id is allocated exactly when it packs below the bound, which reduces the gate on every id based accessor to one acquire load and one compare. Acquiring the bound also acquires the blob install, so the null blob check goes away. It is also the id of the next free slot, which is what iteration wants for its end bound. Drop createSpan with it. It has no callers outside the tests, and it was the only path that could leave a blob partly filled -- it skipped to a fresh blob when a span did not fit, abandoning tail slots that were never handed out and that the packed bound would count as allocated. Without it, blobs fill contiguously and "packs below the bound" means exactly "was handed out". --- include/tsutil/Metrics.h | 81 +++++++++------------- src/records/unit_tests/test_RecRegister.cc | 4 +- src/tsutil/Metrics.cc | 73 +++---------------- src/tsutil/unit_tests/test_Metrics.cc | 54 +-------------- 4 files changed, 50 insertions(+), 162 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index f5cd7763eb7..596a4ce06fa 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -35,8 +35,6 @@ #include #include -#include "swoc/MemSpan.h" - #include "tsutil/Assert.h" namespace ts @@ -85,8 +83,7 @@ class Metrics enum class MetricType : int { COUNTER = 0, GAUGE }; - using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. - using SpanType = swoc::MemSpan; + using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. static constexpr uint16_t MAX_BLOBS = 8192; static constexpr uint16_t MAX_SIZE = 1024; // For a total of 8M metrics @@ -267,9 +264,7 @@ class Metrics iterator end() const { - auto [blob, offset] = _storage->current(); - - return iterator(*this, _makeId(blob, offset, MetricType::COUNTER)); + return iterator(*this, _storage->next_free_id()); } iterator @@ -292,12 +287,6 @@ class Metrics return _storage->create(name, type); } - SpanType - _createSpan(size_t size, MetricType type, IdType *id = nullptr) - { - return _storage->createSpan(size, type, id); - } - // These are little helpers around managing the ID's static constexpr std::tuple _splitID(IdType value) @@ -318,16 +307,29 @@ class Metrics return (t << METRIC_TYPE_BITS | blob << 16 | offset); } + /// A position with no type bits, which is how the next free slot is tracked and compared. + static constexpr uint32_t + _pack(uint16_t blob, uint16_t offset) + { + return static_cast(blob) << 16 | offset; + } + + // _pack must not collide with the type bits, and an offset must fit the field it is packed into. + static_assert(MAX_SIZE <= 0x10000); + static_assert(MAX_BLOBS <= (1 << (METRIC_TYPE_BITS - 16))); + class Storage { - /* _cur_blob and _cur_off are release stored last, after whatever they publish: the blob pointer - * for _cur_blob, the slot's name for _cur_off. Readers acquire load them, _cur_blob first. - * _blobs needs no atomic because it is only read at an index no greater than _cur_blob. - * Writers hold _mutex and load relaxed. + /* The next free slot, packed as @c _makeId would pack it: the blob index above the offset. One + * value rather than two because readers need the pair to be coherent -- a reader that caught a + * new offset against an old blob index, or the reverse, would reject ids that exist or accept + * ids that do not. Release stored last, after whatever it publishes: the blob pointer when it + * crosses a blob, the slot's name otherwise. It only ever increases, so an id is allocated + * exactly when it packs below it. _blobs needs no atomic because it is only read at an index + * this value has published. Writers hold _mutex and load relaxed. */ BlobStorage _blobs; - std::atomic _cur_blob{0}; - std::atomic _cur_off{0}; + std::atomic _next_free{0}; LookupTable _lookups; mutable std::mutex _mutex; @@ -352,14 +354,13 @@ class Metrics AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const; std::string_view name(IdType id) const; MetricType type(IdType id) const; - SpanType createSpan(size_t size, const MetricType type = MetricType::COUNTER, IdType *id = nullptr); bool rename(IdType id, const std::string_view name); - std::pair - current() const + /// The next free slot, as the id it will be given. Also the exclusive bound for iteration. + IdType + next_free_id() const { - std::lock_guard lock(_mutex); - return {_cur_blob.load(std::memory_order_relaxed), _cur_off.load(std::memory_order_relaxed)}; + return static_cast(_next_free.load(std::memory_order_acquire)); } bool @@ -384,13 +385,15 @@ class Metrics auto [blob_ix, offset] = _splitID(id); - // _cur_blob first: acquiring it also makes visible everything published under it. - auto const cur_blob = _cur_blob.load(std::memory_order_acquire); - auto const cur_off = _cur_off.load(std::memory_order_acquire); + // The offset check is not implied by the comparison below: _splitID takes the low 16 bits, so + // an id in an earlier blob can name an offset past MAX_SIZE and still pack below the bound. + if (offset >= MAX_SIZE) { + return false; + } - // A non-null blob past cur_blob is allocated but not yet published, hence <= and < rather - // than a test for "not the current blob". - return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < cur_blob || offset < cur_off); + // Acquiring the bound also makes visible everything published under it, the blob pointer + // included, so _blobs needs no separate check. + return _pack(blob_ix, offset) < _next_free.load(std::memory_order_acquire); } }; @@ -404,7 +407,6 @@ class Metrics { public: using self_type = Gauge; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -480,14 +482,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); } - static Metrics::Gauge::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::GAUGE, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { @@ -522,7 +516,6 @@ class Metrics { public: using self_type = Counter; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -598,14 +591,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); } - static Metrics::Counter::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::COUNTER, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 38fb1adbf03..a406b6fbd6e 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -26,6 +26,7 @@ #include "test_Diags.h" #include +#include #include TEST_CASE("RecRegisterConfig - Type Dispatch", "[librecords][RecConfig]") @@ -103,7 +104,8 @@ TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecL std::atomic finished{false}; std::thread register_metrics([&]() { for (int i = 0; i < 100000; ++i) { - ts::Metrics::Counter::createSpan(1); + // Any registration will do; the point is to grow the store while lookups run. + ts::Metrics::Counter::create("proxy.test.concurrent.reg." + std::to_string(i)); } finished.store(true, std::memory_order_release); }); diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 74ab2be9c54..f0b6f2741ee 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,17 +58,16 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); - auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); + auto const [cur_blob, cur_off] = _splitID(static_cast(_next_free.load(std::memory_order_relaxed))); debug_assert(blob); // The write below is to _blobs[cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. release_assert(cur_blob < MAX_BLOBS - 1); _blobs[cur_blob + 1] = std::move(blob); - _cur_off.store(0, std::memory_order_relaxed); - // Publishes the blob; both writes above are sequenced before it. - _cur_blob.store(cur_blob + 1, std::memory_order_release); + // Publishes the blob and the offset reset as one value; the write above is sequenced before it. + _next_free.store(_pack(cur_blob + 1, 0), std::memory_order_release); } Metrics::IdType @@ -81,11 +80,10 @@ Metrics::Storage::create(std::string_view name, const MetricType type) return it->second; } - // The slot is written below and the bookkeeping only then advances, calling addBlob() once - // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from - // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. - auto const cur_blob = _cur_blob.load(std::memory_order_relaxed); - auto const cur_off = _cur_off.load(std::memory_order_relaxed); + // The slot is written below and the bookkeeping only then advances, calling addBlob() once the + // offset reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from ever + // being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. + auto const [cur_blob, cur_off] = _splitID(static_cast(_next_free.load(std::memory_order_relaxed))); if (cur_blob >= MAX_BLOBS - 1 && cur_off >= MAX_SIZE - 1) { return 0; // Slot 0 is the reserved bad_id. Cannot grow further. @@ -98,11 +96,11 @@ Metrics::Storage::create(std::string_view name, const MetricType type) names[cur_off] = std::make_tuple(std::string(name), id); _lookups.emplace(std::get<0>(names[cur_off]), id); - // Publishes the slot; the name write above is sequenced before it. - _cur_off.store(cur_off + 1, std::memory_order_release); - if (cur_off + 1 >= MAX_SIZE) { - addBlob(); // This resets _cur_off to 0 as well + addBlob(); // Publishes the next blob with a zero offset. + } else { + // Publishes the slot's name. + _next_free.store(_pack(cur_blob, cur_off + 1), std::memory_order_release); } return id; @@ -192,55 +190,6 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } -Metrics::SpanType -Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdType *id) -{ - release_assert(size <= MAX_SIZE); - std::lock_guard lock(_mutex); - - // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow - // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same - // cost: some slots of the last blob go unused. - auto cur_blob = _cur_blob.load(std::memory_order_relaxed); - auto cur_off = _cur_off.load(std::memory_order_relaxed); - - if (cur_blob >= MAX_BLOBS - 1 && cur_off + size >= MAX_SIZE) { - if (id) { - *id = 0; // Slot 0 is the reserved bad_id. - } - return {}; - } - - // A span has to be contiguous, so one that does not fit in the current blob starts a new one. - if (cur_off + size > MAX_SIZE) { - addBlob(); - cur_blob = _cur_blob.load(std::memory_order_relaxed); - cur_off = _cur_off.load(std::memory_order_relaxed); - } - - Metrics::IdType span_start = _makeId(cur_blob, cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); - Metrics::AtomicStorage &atomics = std::get<1>(*blob); - Metrics::SpanType span = Metrics::SpanType(&atomics[cur_off], size); - - if (id) { - *id = span_start; - } - - // Publishes the span's slots. - _cur_off.store(cur_off + size, std::memory_order_release); - - // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending - // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next create() writes one past the - // end of the blob's name array. It also makes end() unreachable for iterator::next(), which - // wraps on ++offset == MAX_SIZE. - if (cur_off + size >= MAX_SIZE) { - addBlob(); - } - - return span; -} - bool Metrics::Storage::rename(Metrics::IdType id, std::string_view name) { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 25dd78c8a49..1913ced12b3 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -95,31 +95,11 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[storeid].load() == 42); } - SECTION("Span allocation") + SECTION("rename") { - ts::Metrics::IdType span_id; - auto fooid = m.lookup("foo"); - auto span = Metrics::Counter::createSpan(17, &span_id); - - REQUIRE(span.size() == 17); - // Not fixed offsets: those only hold against a virgin store. Assert instead that the span - // was allocated above the earlier metric and that every id in it is valid. Both ids are - // counters, so they are directly comparable -- ids encode the metric type, and so are not - // ordered across differing types. - REQUIRE(fooid != ts::Metrics::NOT_FOUND); - REQUIRE(span_id != ts::Metrics::NOT_FOUND); - REQUIRE(span_id > fooid); - for (size_t i = 0; i < span.size(); ++i) { - REQUIRE(m.valid(span_id + static_cast(i))); - } + auto fooid = m.lookup("foo"); - m.rename(span_id + 0, "span.0"); - m.rename(span_id + 1, "span.1"); - m.rename(span_id + 2, "span.2"); - REQUIRE(m.name(fooid) == "foo"); - REQUIRE(m.name(span_id + 0) == "span.0"); - REQUIRE(m.name(span_id + 1) == "span.1"); - REQUIRE(m.name(span_id + 2) == "span.2"); + REQUIRE(fooid != ts::Metrics::NOT_FOUND); m.rename(fooid, "foo-new"); REQUIRE(m.name(fooid) == "foo-new"); REQUIRE(m.lookup("foo") == ts::Metrics::NOT_FOUND); @@ -610,34 +590,6 @@ TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end()); } -TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]") -{ - // A span of MAX_SIZE always lands at offset 0 of an empty blob and fills it, whatever the current - // offset was, so it reaches the blob boundary deterministically. createSpan only targets the - // published store, so this allocates there. - Metrics::IdType span_id = Metrics::NOT_FOUND; - auto span = Metrics::Counter::createSpan(Metrics::MAX_SIZE, &span_id); - - REQUIRE(span.size() == Metrics::MAX_SIZE); - REQUIRE(span_id != Metrics::NOT_FOUND); - REQUIRE(span_id != 0); // 0 is the reserved bad_id, returned only when the store cannot grow. - - // The store must still be usable, and the new metric must be a real, resolvable entry rather - // than something written past the end of a blob. - auto p = Metrics::Counter::createPtr("span.boundary.after"); - REQUIRE(p != nullptr); - - auto &m = Metrics::instance(); - auto id = m.lookup("span.boundary.after"); - REQUIRE(id != Metrics::NOT_FOUND); - REQUIRE(m.valid(id)); - - // And it must behave like any other metric. - Metrics::Counter::increment(p, 7); - REQUIRE(Metrics::Counter::load(p) == 7); - REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p); -} - TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") { // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a malformed one must From 5608493eb04bc6e9f4fb75049b1fc9ba065c55d4 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 2 Sep 2026 17:22:38 -0500 Subject: [PATCH 09/13] Make the concurrent lookup test check what it claims Two ways it could pass without testing anything. Readers only published their tally on exit and nothing made them run before the writer finished, so on one CPU every reader could see stop and resolve nothing while resolved > 0 still held; it now publishes each resolution as it happens and the writer waits for one before stopping. And an id that lookup() clamps resolves to the reserved bad_id slot, whose name is not empty, so the name check could not detect a clamp; it now compares against the name that id must have. --- src/tsutil/unit_tests/test_Metrics.cc | 46 ++++++++++++++++++++------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 1913ced12b3..87f7a1e22f6 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -629,6 +629,7 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M auto &h = Metrics::hidden_instance(); std::atomic stop{false}; + std::atomic ready{0}; std::atomic mismatches{0}; std::atomic resolved{0}; @@ -638,11 +639,21 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M c.store(Metrics::NOT_FOUND, std::memory_order_relaxed); } + // Precomputed so a reader can compare against the name it must see without allocating in the + // loop. Checking the name is the whole point: an id that lookup() clamps resolves to the reserved + // bad_id slot, whose name is not empty, so only the expected name distinguishes the two. + std::vector names; + + names.reserve(N_CREATE); + for (int i = 0; i < N_CREATE; ++i) { + names.push_back("pub.order." + std::to_string(i)); + } + std::vector readers; for (int t = 0; t < N_READERS; ++t) { readers.emplace_back([&]() { - int n = 0; + ready.fetch_add(1, std::memory_order_release); while (!stop.load(std::memory_order_relaxed)) { for (int i = 0; i < N_CREATE; ++i) { @@ -652,27 +663,39 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M continue; } - // valid() accepted the id, so lookup() must hand back the metric with its name rather - // than clamping to the reserved bad_id slot. + // valid() accepted the id, so lookup() must hand back that metric rather than clamping + // to the reserved bad_id slot. std::string_view name; Metrics::MetricType type; auto *m = h.lookup(id, &name, &type); - if (m == nullptr || name.empty()) { + if (m == nullptr || name != names[i]) { mismatches.fetch_add(1, std::memory_order_relaxed); } - ++n; + + // Published as it happens rather than summed at the end, so the writer can wait for it. + resolved.fetch_add(1, std::memory_order_relaxed); } } - resolved.fetch_add(n, std::memory_order_relaxed); }); } + // Every reader has to be in its loop before the writer starts, or the writer can finish and set + // stop before any of them does work, and the test passes without having raced anything. + while (ready.load(std::memory_order_acquire) < N_READERS) { + std::this_thread::yield(); + } + for (int i = 0; i < N_CREATE; ++i) { - auto const nm = "pub.order." + std::to_string(i); + REQUIRE(Metrics::Counter::createHiddenPtr(names[i]) != nullptr); + created[i].store(h.lookup(names[i]), std::memory_order_relaxed); + } - REQUIRE(Metrics::Counter::createHiddenPtr(nm) != nullptr); - created[i].store(h.lookup(nm), std::memory_order_relaxed); + // Readers being in their loops is not enough to guarantee they did any work: on a single CPU the + // writer can run to completion first, and every reader would then see stop and resolve nothing. + // Wait for one actual resolution so the check below cannot pass vacuously. + while (resolved.load(std::memory_order_relaxed) == 0) { + std::this_thread::yield(); } stop.store(true, std::memory_order_relaxed); @@ -687,12 +710,11 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M auto hi = std::numeric_limits::min(); for (int i = 0; i < N_CREATE; ++i) { - auto const nm = "pub.order." + std::to_string(i); - auto const id = h.lookup(nm); + auto const id = h.lookup(names[i]); REQUIRE(id != Metrics::NOT_FOUND); REQUIRE(h.valid(id)); - REQUIRE(h.name(id) == nm); + REQUIRE(h.name(id) == names[i]); lo = std::min(lo, id); hi = std::max(hi, id); From 580b4d13de57249b568bef756f4cb9a286ce5c74 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 2 Sep 2026 19:19:11 -0500 Subject: [PATCH 10/13] Include and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied for NOT_FOUND's numeric_limits. The header still compiles, through some other transitive path, which is exactly what makes it worth declaring. addBlob() destructured the packed value but only ever used the blob half. --- include/tsutil/Metrics.h | 1 + src/tsutil/Metrics.cc | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 596a4ce06fa..e689d433ef2 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index f0b6f2741ee..ccae6475542 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,7 +58,8 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); - auto const [cur_blob, cur_off] = _splitID(static_cast(_next_free.load(std::memory_order_relaxed))); + // Only the blob index is needed; the offset resets to zero below. + auto const cur_blob = static_cast(_next_free.load(std::memory_order_relaxed) >> 16); debug_assert(blob); // The write below is to _blobs[cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. From f0478d5819e80bd4ef8c35659f5a3b6e96945b13 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Thu, 3 Sep 2026 10:21:42 -0500 Subject: [PATCH 11/13] Take the lock before touching a slot's name in rename() The name is the key _lookups is indexed by, so replacing it belongs entirely inside the lock. Nothing read the string outside it before -- binding a reference to it does not touch its bytes -- but computing that reference outside the lock made the boundary look wider than it is, and there is no reason for anything here to sit outside. --- src/tsutil/Metrics.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index ccae6475542..e2b6fd750ad 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -199,11 +199,13 @@ Metrics::Storage::rename(Metrics::IdType id, std::string_view name) return false; } + // Held across the whole rename: the name is the key _lookups is indexed by, so replacing it has + // to be serialized against every other writer of that slot's name. + std::lock_guard lock(_mutex); + auto [blob_ix, offset] = _splitID(id); Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); - - std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); - std::lock_guard lock(_mutex); + std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); if (cur.length() > 0) { _lookups.erase(cur); From 8a7e9c3b3e035fd4534d31f68e0c994f92d7ef2b Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Thu, 3 Sep 2026 13:01:36 -0500 Subject: [PATCH 12/13] Metrics: trim comments, and drop ones about a check that is gone Two comments in the malformed-offset test explained that the null blob check, not the offset test, would reject those ids with only one blob allocated. The packed bound removed that check, so the reasoning no longer applied. --- include/tsutil/Metrics.h | 23 +++++++++-------------- src/tsutil/Metrics.cc | 8 +++----- src/tsutil/unit_tests/test_Metrics.cc | 24 +++++++++--------------- 3 files changed, 21 insertions(+), 34 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index e689d433ef2..ec9d9e58dda 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -308,26 +308,23 @@ class Metrics return (t << METRIC_TYPE_BITS | blob << 16 | offset); } - /// A position with no type bits, which is how the next free slot is tracked and compared. + /// As @c _makeId, without the type bits. static constexpr uint32_t _pack(uint16_t blob, uint16_t offset) { return static_cast(blob) << 16 | offset; } - // _pack must not collide with the type bits, and an offset must fit the field it is packed into. + // A packed position must not reach the type bits, and an offset must fit its field. static_assert(MAX_SIZE <= 0x10000); static_assert(MAX_BLOBS <= (1 << (METRIC_TYPE_BITS - 16))); class Storage { - /* The next free slot, packed as @c _makeId would pack it: the blob index above the offset. One - * value rather than two because readers need the pair to be coherent -- a reader that caught a - * new offset against an old blob index, or the reverse, would reject ids that exist or accept - * ids that do not. Release stored last, after whatever it publishes: the blob pointer when it - * crosses a blob, the slot's name otherwise. It only ever increases, so an id is allocated - * exactly when it packs below it. _blobs needs no atomic because it is only read at an index - * this value has published. Writers hold _mutex and load relaxed. + /* The next free slot, packed as @c _makeId packs one. A single value because a reader that + * caught a new offset against an old blob index, or the reverse, would reject ids that exist + * or accept ids that do not. Release stored last, after the blob pointer or the slot's name it + * publishes. Only ever increases, so an id is allocated exactly when it packs below it. */ BlobStorage _blobs; std::atomic _next_free{0}; @@ -357,7 +354,7 @@ class Metrics MetricType type(IdType id) const; bool rename(IdType id, const std::string_view name); - /// The next free slot, as the id it will be given. Also the exclusive bound for iteration. + /// The id the next slot will get, which is also iteration's exclusive bound. IdType next_free_id() const { @@ -386,14 +383,12 @@ class Metrics auto [blob_ix, offset] = _splitID(id); - // The offset check is not implied by the comparison below: _splitID takes the low 16 bits, so - // an id in an earlier blob can name an offset past MAX_SIZE and still pack below the bound. + // Not implied below: an earlier blob can name an offset past MAX_SIZE and still pack under. if (offset >= MAX_SIZE) { return false; } - // Acquiring the bound also makes visible everything published under it, the blob pointer - // included, so _blobs needs no separate check. + // Acquiring the bound acquires the blob install, so _blobs needs no check of its own. return _pack(blob_ix, offset) < _next_free.load(std::memory_order_acquire); } }; diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index e2b6fd750ad..eaace85928f 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,7 +58,6 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); - // Only the blob index is needed; the offset resets to zero below. auto const cur_blob = static_cast(_next_free.load(std::memory_order_relaxed) >> 16); debug_assert(blob); @@ -67,7 +66,7 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! _blobs[cur_blob + 1] = std::move(blob); - // Publishes the blob and the offset reset as one value; the write above is sequenced before it. + // One store publishes both; the install above is sequenced before it. _next_free.store(_pack(cur_blob + 1, 0), std::memory_order_release); } @@ -98,7 +97,7 @@ Metrics::Storage::create(std::string_view name, const MetricType type) _lookups.emplace(std::get<0>(names[cur_off]), id); if (cur_off + 1 >= MAX_SIZE) { - addBlob(); // Publishes the next blob with a zero offset. + addBlob(); } else { // Publishes the slot's name. _next_free.store(_pack(cur_blob, cur_off + 1), std::memory_order_release); @@ -199,8 +198,7 @@ Metrics::Storage::rename(Metrics::IdType id, std::string_view name) return false; } - // Held across the whole rename: the name is the key _lookups is indexed by, so replacing it has - // to be serialized against every other writer of that slot's name. + // The name is the key _lookups is indexed by, so the whole replacement is serialized. std::lock_guard lock(_mutex); auto [blob_ix, offset] = _splitID(id); diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 87f7a1e22f6..3995516bdf9 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -593,8 +593,8 @@ TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") { // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a malformed one must - // not index past a blob's arrays. Two blobs are needed for the offset check to be what rejects - // it; with one, the null blob check would. + // not index past a blob's arrays. Filling past one blob puts the ids below in earlier blobs, + // where they pack under the bound and only the MAX_SIZE test rejects them. auto &h = Metrics::hidden_instance(); for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) { @@ -604,8 +604,6 @@ TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics] auto const *bad = h.lookup(Metrics::IdType{0}); // the reserved bad_id slot REQUIRE(bad != nullptr); - // blob 0 is allocated, so the null check does not fire; only the MAX_SIZE test stands between - // this and atomics[65535]. for (Metrics::IdType id : {Metrics::IdType{0x0000FFFF}, Metrics::IdType{0x00000400}, Metrics::IdType{0x0001FFFF}}) { REQUIRE(h.valid(id) == false); REQUIRE(h.lookup(id) == bad); @@ -639,9 +637,8 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M c.store(Metrics::NOT_FOUND, std::memory_order_relaxed); } - // Precomputed so a reader can compare against the name it must see without allocating in the - // loop. Checking the name is the whole point: an id that lookup() clamps resolves to the reserved - // bad_id slot, whose name is not empty, so only the expected name distinguishes the two. + // A clamped lookup resolves to the bad_id slot, whose name is not empty, so only the expected + // name detects one. std::vector names; names.reserve(N_CREATE); @@ -663,8 +660,7 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M continue; } - // valid() accepted the id, so lookup() must hand back that metric rather than clamping - // to the reserved bad_id slot. + // valid() accepted the id, so lookup() must return that metric and not clamp. std::string_view name; Metrics::MetricType type; auto *m = h.lookup(id, &name, &type); @@ -673,15 +669,14 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M mismatches.fetch_add(1, std::memory_order_relaxed); } - // Published as it happens rather than summed at the end, so the writer can wait for it. + // Published as it happens so the writer can wait for one. resolved.fetch_add(1, std::memory_order_relaxed); } } }); } - // Every reader has to be in its loop before the writer starts, or the writer can finish and set - // stop before any of them does work, and the test passes without having raced anything. + // Readers must be in their loops before the writer starts, or nothing races. while (ready.load(std::memory_order_acquire) < N_READERS) { std::this_thread::yield(); } @@ -691,9 +686,8 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M created[i].store(h.lookup(names[i]), std::memory_order_relaxed); } - // Readers being in their loops is not enough to guarantee they did any work: on a single CPU the - // writer can run to completion first, and every reader would then see stop and resolve nothing. - // Wait for one actual resolution so the check below cannot pass vacuously. + // Being in the loop is not doing work: on one CPU the writer can finish first and every reader + // would then see stop. Wait for a real resolution so the check below cannot pass vacuously. while (resolved.load(std::memory_order_relaxed) == 0) { std::this_thread::yield(); } From 2d05898c94eb8bdde289392cc2f0d7acd96bcec4 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Thu, 3 Sep 2026 15:45:34 -0500 Subject: [PATCH 13/13] Remove rename() It mutated a slot's name while name() and lookup(id, &out_name) read that same std::string without the mutex and hand out views into it, which moonchen reproduced as a TSAN race. Locking rename() does not fix it; the readers are the lock free paths this PR exists to keep. Giving names immutable storage with its own lifetime rules would, but nothing outside the tests calls rename(). Without it a name is written once before the store that publishes it and never changes, so those readers are correct by construction. --- include/tsutil/Metrics.h | 10 +++------- src/tsutil/Metrics.cc | 24 ------------------------ src/tsutil/unit_tests/test_Metrics.cc | 11 ----------- 3 files changed, 3 insertions(+), 42 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index ec9d9e58dda..741b1e069a5 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -145,12 +145,6 @@ class Metrics { return _storage->lookup(id, out_name, type); } - bool - rename(IdType id, const std::string_view name) - { - return _storage->rename(id, name); - } - AtomicType & operator[](IdType id) { @@ -325,6 +319,9 @@ class Metrics * caught a new offset against an old blob index, or the reverse, would reject ids that exist * or accept ids that do not. Release stored last, after the blob pointer or the slot's name it * publishes. Only ever increases, so an id is allocated exactly when it packs below it. + * + * A slot's name is written once, before the store that publishes it, and never changes, which + * is what lets @c name and @c lookup hand out a view of it without the mutex. */ BlobStorage _blobs; std::atomic _next_free{0}; @@ -352,7 +349,6 @@ class Metrics AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const; std::string_view name(IdType id) const; MetricType type(IdType id) const; - bool rename(IdType id, const std::string_view name); /// The id the next slot will get, which is also iteration's exclusive bound. IdType diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index eaace85928f..1168c7d2e44 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -190,30 +190,6 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } -bool -Metrics::Storage::rename(Metrics::IdType id, std::string_view name) -{ - // We can only rename Metrics that are already allocated - if (!_is_allocated(id)) { - return false; - } - - // The name is the key _lookups is indexed by, so the whole replacement is serialized. - std::lock_guard lock(_mutex); - - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); - std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); - - if (cur.length() > 0) { - _lookups.erase(cur); - } - cur = name; - _lookups.emplace(cur, id); - - return true; -} - // Iterator implementation void Metrics::iterator::next() diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 3995516bdf9..f267097807f 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -95,17 +95,6 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[storeid].load() == 42); } - SECTION("rename") - { - auto fooid = m.lookup("foo"); - - REQUIRE(fooid != ts::Metrics::NOT_FOUND); - m.rename(fooid, "foo-new"); - REQUIRE(m.name(fooid) == "foo-new"); - REQUIRE(m.lookup("foo") == ts::Metrics::NOT_FOUND); - REQUIRE(m.lookup("foo-new") == fooid); - } - SECTION("lookup") { auto nm = m.lookup("notametric");