cmcfarlen commented on PR #13583: URL: https://github.com/apache/trafficserver/pull/13583#issuecomment-5517270871
Pushed two commits for this. **The blob rollover — 96e8e5b42e** You were right that publishing one coherent value is the fix, and reordering the stores is not. Writing `_cur_blob` first and `_cur_off` second only trades a false reject for a false accept: a reader then sees the new blob with the old offset and accepts ids in a blob nothing has been written to. Two independent atomics have no coherent pair in either order. `_cur_blob` and `_cur_off` are now one `std::atomic<uint32_t>` holding the blob index above the offset, packed exactly as `_makeId` packs them. Crossing a blob is a single release store: ```cpp _next_free.store(_pack(cur_blob + 1, 0), std::memory_order_release); ``` Three things fall out of it: - The value only ever increases — `(N+1)<<16` exceeds `N<<16 + offset` for any `offset < MAX_SIZE` — so an id is allocated exactly when it packs below the bound. `_is_allocated()` becomes one acquire load and one compare. - Acquiring the bound acquires the blob install that was release-ordered before it, so the `_blobs[blob_ix] != nullptr` check is redundant and gone. That also answers the review note about the second acquire load on the hot path: there is only one now. - The packed value *is* the COUNTER-typed id of the next free slot, so iteration's end bound is the load rather than something reconstructed from two fields. The `offset >= MAX_SIZE` check has to stay. `_splitID` takes the low 16 bits, so an id in an earlier blob can name offset 60000 and still pack below the bound. **createSpan is gone with it** It had no callers outside the tests, and it was the only path that could leave a blob partly filled: when a span did not fit it skipped to a fresh blob, abandoning tail slots that were never handed out. Those slots pack below the bound, so the packed check would have called them allocated — which is also the substance of the suppressed review comment about `offset < MAX_SIZE` in earlier blobs. With `createSpan` gone, blobs fill contiguously and "packs below the bound" means exactly "was handed out", with no special case. Two test consumers adapted: the span/rename section became a rename-only section, and `test_RecRegister.cc` was using `createSpan(1)` as a cheap anonymous registration while hammering lookups, which `create()` does just as well. The test case that existed only to cover `createSpan`'s blob-boundary quirk is deleted. Note this removes a function from an installed header. Given it hands out unnamed slots and is unused in tree, out-of-tree use seems unlikely, but it is worth a line in the release notes. **The test problems — 5608493eb0** Both real, and the barrier alone would not have fixed the first one. A ready-count gets every reader into its loop, but on a single CPU the writer can still run to completion before any reader is scheduled, and they would all then see `stop` and resolve nothing while `resolved > 0` still passed. So readers now publish each resolution as it happens and the writer waits for one before setting `stop`. The ready-count is in there too, so the race still happens when there are CPUs for it. The name check could not detect a clamp, as you said — `bad_id`'s name is not empty. Names are precomputed before the threads start and the reader compares against the one that id must have, so no allocation in the loop. Full `test_tsutil` and `test_records` pass, and I checked both commits build individually. -- 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]
