phongn commented on PR #13170: URL: https://github.com/apache/trafficserver/pull/13170#issuecomment-5374053377
A follow-up on the `atomic_ref` prototype, because it deserves a direct answer to the strongest objection to any lock-free fix here: **what stops a popper from reading memory the list no longer owns?** An external allocator does not synchronize with a lock-free reader, so if a popped node can be returned to an allocator that unmaps it, a stale reader can fault, and no atomic access fixes a page fault. That is the memory-reclamation problem, and the general solutions (hazard pointers, epochs) are heavyweight — or, as a minimal reader-indication scheme, end up resembling a lock. The prototype does not solve the general problem. It relies on two narrower invariants, one structural and one already load-bearing in master and in this PR: **1. `InkFreeList`: the memory is never returned to any allocator.** Chunks are allocated once and are immortal — in master, in this PR, and in the prototype. There is no "memory the freelist no longer owns"; a caller holding an object still sits inside a live chunk. A stale popper's relaxed read can only observe garbage from a caller's live object, which the versioned CAS rejects. This is the type-stable-memory setting the Treiber stack was designed for (same reasoning as the kernel's `SLAB_TYPESAFE_BY_RCU`). No reclamation machinery is needed because there is no reclamation. Notably, the freelist is where the measured regression lives — `benchmark_FreeList` exercises only `InkFreeList`. **2. `InkAtomicList`: the hazard requires two threads popping the same list concurrently.** A stale reader can only exist if another pop races it. An audit of every consumer in the tree shows all atomic lists are multi-producer/single-consumer: `NetHandler` drains the enable lists on the owning net thread (`NetHandler.cc:228,238`), the AIO thread drains `aio_todo`/`aio_temp_list` for its fd (`AIO.cc:362,513`), the UDP net thread drains its queues (`UnixUDPNet.cc:1341,1885`), `QUICClosedConCollector` drains `closedQueue` on its own continuation, each `LogBufferManager` is drained by its flush thread, and each `ProtectedQueue` by its ethread. Single consumer means no concurrent pop, means the reclamation window is unreachable — which is also why master's racy version never faulted in 25 years of production. So the honest framing of lock-vs-prototype is not "correct vs. fast": both are correct for the code as it exists. The difference is **guarantee versus discipline**. The lock makes `InkAtomicList` safe for any future caller, including a hypothetical multi-consumer list whose items go to `free()`; the prototype keeps the discipline implicit, exactly as master always has. Two considerations for that judgment: * This PR already relies on the same discipline: `ink_atomiclist_remove` is not locked, so a remove racing a locked pop reopens the identical window. It is safe today because removers are the sole poppers of their lists — i.e., the single-consumer discipline is load-bearing either way. * There is a middle path that prices the disagreement cleanly: **lock-free freelist** (structurally safe, and where the uncontended regression is) **plus locked `InkAtomicList` pops** (caller-owned memory keeps the hard guarantee). And the discipline can become a *checked* discipline for free: a DEBUG-only assertion that records the first popper's thread ID per list and asserts subsequent pops match. That is a stronger guardrail than master ever had, at zero production cost. If ATS ever genuinely needs multi-consumer atomic lists over memory that can be unmapped, the escalation path is the reader-indication scheme sketched in this PR's description (a per-popper announce slot that a reclaiming thread waits on) — a single-slot hazard pointer. Nothing today needs it. Either way, the deciding number is end-to-end: if the regression seen in production benchmarking is the uncontended mutex tax (the 1-thread microbenchmarks strongly suggest it is), the prototype — or the hybrid — should recover most of it while keeping this PR's UB fixes intact. -- 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]
