Metrics: Revert added locking and annotations by moonchen · Pull Request #13567 · apache/trafficserver

moonchen · 2026-08-18T22:41:47Z

The locking that the thread safety annotations required on the ts::Metrics::Storage read paths introduces too much of a performance regression. Revert it for now, until a better solution can be found.

The thread safety analysis infrastructure and its use in SSLOriginSessionCache are unaffected.

The locking that the thread safety annotations required on the ts::Metrics::Storage read paths introduces too much of a performance regression. Revert it for now, until a better solution can be found. The thread safety analysis infrastructure and its use in SSLOriginSessionCache are unaffected.

Copilot

Pull request overview

This PR updates the ts::Metrics::Storage implementation to roll back previously-added thread-safety annotations and associated locking on some read paths, aiming to avoid a performance regression in metric reads while keeping the rest of the thread-safety infrastructure (e.g., SSLOriginSessionCache) unchanged.

Changes:

Replaced ts::mutex / ts::lock_guard usage in Metrics::Storage with std::mutex / std::lock_guard.
Removed several thread-safety annotations (TS_GUARDED_BY, TS_REQUIRES, TS_NO_THREAD_SAFETY_ANALYSIS) from Metrics::Storage.
Removed locking in some Storage read helpers (lookup(id, ...), name(id), and valid(id)), and adjusted rename() locking.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/tsutil/Metrics.cc`	Adjusts locking strategy around `Storage` operations, including removing locks from some ID-based read paths.
`include/tsutil/Metrics.h`	Removes thread-safety annotations and switches `Storage` to `std::mutex`, including changing/relaxing locking in small inline helpers.

Suppressed comments (2)

src/tsutil/Metrics.cc:145

Storage::name() reads _cur_blob, _cur_off, and _blobs[...] without synchronization, but those are updated under _mutex in create() / createSpan(). This introduces a data race and can lead to returning the wrong name or dereferencing a partially-published blob under concurrent metric creation.

Metrics::Storage::name(Metrics::IdType id) const
{
  auto [blob_ix, offset]         = _splitID(id);
  Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();

src/tsutil/Metrics.cc:200

Storage::rename() acquires _mutex only after reading _blobs[...], _cur_blob, _cur_off, and taking a reference to the stored name. With concurrent create() / createSpan() this is a data race, and even without it the late lock undermines the function’s own correctness guarantees. Take the lock before accessing any shared state and remove the later lock_guard.

  std::string    &cur = std::get<0>(std::get<0>(*blob)[offset]);
  std::lock_guard lock(_mutex);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics::MetricType *out_type) const
 {
-  ts::lock_guard lock(_mutex);
  auto [blob_ix, offset]         = _splitID(id);
  Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();



    valid(IdType id) const
    {
      auto [blob, entry] = _splitID(id);

-      ts::lock_guard lock(_mutex);
      return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off)));


cmcfarlen

Thanks! I know copilot is not happy about it, but we can address the "data race" with atomics in a followup PR.

cmcfarlen · 2026-08-19T17:42:43Z

Cherry-picked to 10.2.x

The locking that the thread safety annotations required on the ts::Metrics::Storage read paths introduces too much of a performance regression. Revert it for now, until a better solution can be found. The thread safety analysis infrastructure and its use in SSLOriginSessionCache are unaffected. (cherry picked from commit 0201367)

…vert (#13583) * 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. * 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. * 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. * 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. * 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. * 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. * 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. * 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". * 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. * Include <limits> and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied <limits> 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. * 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. * 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. * 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.

…vert (apache#13583) * 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. * Metrics: publish the allocation point with release/acquire The lock removal in apache#13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race apache#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. * 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. * 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. * 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. * 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. * 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. * 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". * 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. * Include <limits> and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied <limits> 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. * 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. * 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. * 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. (cherry picked from commit c7af2e3)

Copilot AI lite review requested due to automatic review settings August 18, 2026 22:41

moonchen added the Backport Marked for backport for an LTS patch release label Aug 18, 2026

Copilot started reviewing on behalf of moonchen August 18, 2026 22:42 View session

moonchen self-assigned this Aug 18, 2026

moonchen added Regression Revert Metrics Performance and removed Backport Marked for backport for an LTS patch release labels Aug 18, 2026

moonchen added this to ATS v10.2.x Aug 18, 2026

moonchen requested a review from cmcfarlen August 18, 2026 22:44

moonchen added this to the 11.0.0 milestone Aug 18, 2026

Copilot AI reviewed Aug 18, 2026

View reviewed changes

cmcfarlen approved these changes Aug 18, 2026

View reviewed changes

moonchen merged commit 0201367 into apache:master Aug 19, 2026
15 checks passed

github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Aug 19, 2026

cmcfarlen moved this from For v10.2.0 to Picked v10.2.1 in ATS v10.2.x Aug 19, 2026

cmcfarlen modified the milestones: 11.0.0, 10.2.1 Aug 19, 2026

cmcfarlen mentioned this pull request Aug 21, 2026

Metrics: close the id lookup race and bounds gaps left by the lock revert #13583

Merged

Navigation Menu

Uh oh!

Metrics: Revert added locking and annotations - #13567

Metrics: Revert added locking and annotations#13567
moonchen merged 1 commit into
apache:masterfrom
moonchen:revert-metrics-thread-safety-locks

moonchen commented Aug 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

cmcfarlen left a comment

Uh oh!

Uh oh!

cmcfarlen commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

moonchen commented Aug 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

cmcfarlen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmcfarlen commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants