cmcfarlen opened a new pull request, #13583: URL: https://github.com/apache/trafficserver/pull/13583
Follow-ups to #13567, which reverted the locking that #13310 had added to the `ts::Metrics::Storage` read paths. That revert restored the performance but left the data race #13310 was closing, plus some pre-existing bounds problems in the same functions. This closes the race without a lock, and fixes the bounds. ### One gate for id validation `valid()`, `lookup(IdType)`, `name()` and `rename()` each carried their own copy of the same range test, and the copies disagreed. `valid()` rejected an offset past `MAX_SIZE`; the other three did not. `_splitID` passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, so 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()`. It 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. ### Publication, without a lock `_cur_blob` and `_cur_off` become atomics, but the point is not that each is atomic — that alone would not make the pair update atomically. They are publication points: each is release stored last, after whatever it makes visible (the blob pointer and the reset offset for `_cur_blob`, the slot's name for `_cur_off`), and a reader acquire loads `_cur_blob` first. Observing a value for it also observes everything written before it was released, so 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. Neither a packed word nor per-blob counters are needed. `addBlob()` also writes the blob pointer and resets `_cur_off` before advancing `_cur_blob`, so `_cur_blob` alone publishes the blob. The blob comparison in `_is_allocated` is `<=` / `<` rather than a test for "not the current blob" to match, since a blob past `_cur_blob` can be allocated but unpublished. `_blobs` needs no 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. 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. ### `_extractType` on a negative id It 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 sufficient on its own: the sign bit sits above the type field, so `NOT_FOUND` still yields `4`. Masking to the single bit `_makeId` writes makes the function total for any input. ### Testing A new test resolves ids from several threads while another registers metrics across a few blob boundaries. Under the `tsan` preset, making either allocation counter non-atomic again reports a data race there. It does **not** catch a downgrade of the release/acquire pairs to relaxed — atomics are race free at any ordering, so TSAN stays quiet and the assertions still hold. The memory orders are reviewed, not tested, and the test says so. `tools/benchmark/benchmark_Metrics.cc` is new; nothing in tree measured these paths, which is how a global mutex on the hottest one went unnoticed. Four cases scaled by thread count, on a 10 core machine at 20k ops/thread: | threads | `increment(ptr)` | `increment(id)` | `lookup(id)` | `lookup(name)` | |---|---|---|---|---| | 1 | 0.12 ms | 1.81 ms | 1.05 ms | 2.34 ms | | 4 | 0.24 ms | 2.20 ms | 1.15 ms | 37.6 ms | | 16 | 4.71 ms | 7.04 ms | 2.09 ms | 80.4 ms | | 64 | 19.6 ms | 24.0 ms | 6.13 ms | 317 ms | `lookup(name)` is a deliberate control: it still takes the mutex, so it must degrade with thread count. It goes 2.3 ms to 317 ms while `lookup(id)` goes 1.05 to 6.13 ms, which is the evidence that the harness loads the machine rather than the lock free numbers being flat for want of load. Above 10 threads the machine is oversubscribed, so treat the shape as meaningful and the magnitudes as not. Comparing a build with and without the atomics commit put every case within noise, the only consistent signal being 4-8% on `lookup(id)` — two `ldaprh` rather than two `ldrh` on ARM64, and plain loads on x86. Set against what the mutex costs, it is not a trade worth considering. ### Provenance The bounds and memory-order findings came out of a review of this code prompted by a production `perf` profile, in which the `#13310` locking accounted for roughly half of all CPU in futex contention. I do not have a public link for that review to cite. The parts of it this PR does not implement — deleting the `sdk_assert` from the `TSStat*` entry points, an opaque handle API for plugins, restructuring `Storage` around per-blob published counts, and removing `rename()` — were either out of proportion to the measured benefit or need an upstream decision first. `createSpan`'s blob boundary and `addBlob`'s bound assert were part of the same review and landed earlier in #13505. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
