From 91347a96ed691927ed1906f01c11b3587cdfd0a8 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 19 Aug 2026 09:16:18 -0500 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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; });