Convert ink_queue implementation to std::atomic

[JosiahWI]

The PR title is fairly self-explanatory, but the design choices here deserve explicit mention.

• There is a new TS_HAS_128BIT_ATOMIC so that 128-bit head objects depends on both __sync and __atomic

In our build pipeline, OSX and FreeBSD support this.

• The head_p type is no longer a union with a {pointer, version} field

This has been done to eliminate type punning, which was done all over the implementation, and is UB. The pointer and version are now always set through the macros FREELIST_POINTER, FREELIST_VERSION, and SET_FREELIST_POINTER_VERSION, which use a separate {pointer, version} struct type and memcpy on platforms where this is appropriate (see preprocessor defs for the list).

• The head_p type has been changed into a type alias of the data type

This is necessary so that the head of the list will be an atomic integer type instead of an atomic class type to be sure it can use 128bit atomic hardware instructions on platforms that support them.

• Freelist alignment is now adjusted to be satisfy void* alignment requirements

This is a minor bug in master.

• The freelist and atomiclist pop operations (called freelist_new for the freelist) are now locked to provide mutual exclusion

This particular change was made to fully fix #11640 - there is a minor data race without it in that the second pointer from the list head can be overwritten by an allocator's placement new before it is read without synchronization in freelist_new (a similar argument applies to atomiclist_pop; atomiclist_popall is unaffected) by another thread, which is going to subsequently find out the list head is stale and retry. Thus, the garbage pointer is not dereferenced, but this is still UB. Benchmarking suggests this has not caused any performance regression (see below). In fact, I wonder if the addition of the lock helped performance under contention.

I have been thinking about other approaches here. One approach is to add an atomic flag to the list head that is set by any thread popping from the list. A thread that has successfully popped can spin on that flag to wait for the completion of any other threads still reading the memory it is about to return. My intuition is that this will be better, but I don't know without benchmarking, and it's a lot more complex than the lock.

Fixes #7398
Fixes #11640 in release mode only (dummy_forced_read calls still race)
Partially, #13572

Previous Work

See #7382. This PR is only a step in the direction of #7382; it retains a lot of the old code structure along with most of its design flaws. If this change is accepted, it should thereafter be possible to apply other design improvements from #7382, such as the fleshed out versioned pointer type, with greater confidence.

A Few Comments About Assertions

This PR adds a hoard of assertions that check alignment requirements. Most of them are debug assertions - the alignment check on the pointer passed to freelist_push is a release assert for now, because it would almost certainly indicate a major issue if it triggered. According to the comment from @bryancall, this assertion was in fact failing before (it was previously a debug assert, and he commented it out). I am hopeful that that issue is now resolved.

Performance Implications (from tools/benchmark/benchmark_FreeList)

This change represents a significant performance improvement as number of threads increases. The following benchmarks are from tools/benchmark/benchmark_FreeList. The first one is from the old code without this patch. The second one is a benchmark of the code with this patch, but the size of the head_p pointer is different because my system does not support 128bit atomics in hardware.

Before (without this PR) (-DCMAKE_BUILD_TYPE=Release, 128 bit head_p)

nthreads = 1                                   100             1     3.43135 s
                                        34.1753 ms    34.0414 ms      34.32 ms
                                        708.547 us    634.286 us    798.867 us

nthreads = 2                                   100             1     48.6705 s
                                        362.513 ms      336.2 ms    388.466 ms
                                        133.529 ms    124.707 ms     143.15 ms

nthreads = 6                                   100             1     2.44896 m
                                         1.44199 s     1.43396 s     1.45083 s
                                        42.9103 ms    37.2258 ms    50.7251 ms

After (with this PR) (-DCMAKE_BUILD_TYPE=Release, 64 bit head_p)

benchmark name                       samples       iterations    est run time
                                     mean          low mean      high mean
                                     std dev       low std dev   high std dev
-------------------------------------------------------------------------------
nthreads = 1                                   100             1     3.11346 s
                                        30.9922 ms    30.8757 ms    31.1318 ms
                                        646.479 us    546.436 us     845.25 us

nthreads = 2                                   100             1     19.0999 s
                                        182.391 ms      180.4 ms    183.905 ms
                                        8.80696 ms    6.94193 ms     13.516 ms
                                        
nthreads = 6                                   100             1     1.50301 m
                                        975.659 ms     960.88 ms    990.707 ms
                                        75.9773 ms    71.2177 ms    81.3398 ms

[Copilot]

Pull request overview

This PR refactors the ink_queue freelist / atomic list implementation to use std::atomic-based state (including a revised head_p representation) and adds unit tests/benchmarks to validate and measure the behavior. The goal is to eliminate UB from type punning and improve correctness around alignment and concurrency.

Changes:

• Refactor head_p to an integral type and introduce memcpy-based view/load/store helpers for pointer+version packing.
• Update freelist/atomiclist operations to use std::atomic (and add mutex-based mutual exclusion for pop paths).
• Add Catch2 unit tests/benchmarks for freelist and atomic list behavior, and update build configuration accordingly.

Reviewed changes

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

Show a summary per file

File	Description
include/tscore/ink_queue.h	Refactors head_p, adds atomic/mutex members to list types, and updates pointer/version access macros.
src/tscore/ink_queue.cc	Migrates freelist/atomiclist logic to std::atomic + new packing helpers; adds alignment checks and mutexes.
src/tscore/unit_tests/test_ink_queue.cc	New Catch2 unit tests and benchmarks for freelist/atomic list behavior.
src/tscore/CMakeLists.txt	Adds the new unit test and links atomic.
src/proxy/logging/LogObject.cc	Adapts CAS usage / version typing to the new head_p API.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

cmake/Check128BitCas.cmake:45

• CHECK_PROGRAM as written will not compile as C++: std::atomic<__int128>::compare_exchange_strong takes expected by reference and desired as the second parameter, but here both arguments are rvalues and reversed. Also this file includes CheckCSourceCompiles but calls check_cxx_source_compiles, and the fallback uses check_c_source_compiles even though the program is C++ (<atomic>). Please switch to include(CheckCXXSourceCompiles) and use a valid C++ compare-exchange snippet (with a mutable expected) for both checks (including the -mcx16 probe).

set(CHECK_PROGRAM
    "
    #include <atomic>

    int main()
    {
        std::atomic<__int128> x{0};
        return x.compare_exchange_strong(10, 0);
    }
    "
)

include(CheckCSourceCompiles)
check_cxx_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS)

if(NOT TS_HAS_128BIT_CAS)
  unset(TS_HAS_128BIT_CAS CACHE)
  set(CMAKE_REQUIRED_FLAGS "-Werror -mcx16")
  check_c_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS)
  set(NEED_MCX16 ${TS_HAS_128BIT_CAS})

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

include/tscore/ink_queue.h:250

• INK_ATOMICLIST_EMPTY still passes the std::atomic<head_p> member directly into FREELIST_POINTER/TO_PTR. Since std::atomic is not implicitly convertible to head_p, this will not compile (and call sites like ProtectedQueue rely on this macro). Load the atomic (e.g., use .head.load(...)) before applying FREELIST_POINTER/TO_PTR, and update both NT/non-NT macro branches accordingly.

#if !defined(INK_QUEUE_NT)
#define INK_ATOMICLIST_EMPTY(_x) (!(TO_PTR(FREELIST_POINTER((_x.head)))))
#else
/* ink_queue_nt.c doesn't do the FROM/TO pointer swizzling */
#define INK_ATOMICLIST_EMPTY(_x) (!((FREELIST_POINTER((_x.head)))))
#endif

cmake/Check128BitCas.cmake:46

• Check128BitCas.cmake calls check_cxx_source_compiles but only includes CheckCSourceCompiles, which does not define that macro in standard CMake. Also, the fallback path still uses check_c_source_compiles even though CHECK_PROGRAM is now C++ (includes , std::atomic), so the -mcx16 probe will always fail. Include CheckCXXSourceCompiles and use check_cxx_source_compiles for both probes.

include(CheckCSourceCompiles)
check_cxx_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS)

if(NOT TS_HAS_128BIT_CAS)
  unset(TS_HAS_128BIT_CAS CACHE)
  set(CMAKE_REQUIRED_FLAGS "-Werror -mcx16")
  check_c_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS)
  set(NEED_MCX16 ${TS_HAS_128BIT_CAS})

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

include/tscore/ink_queue.h:256

• INK_ATOMICLIST_EMPTY reads InkAtomicList::head without an explicit .load(). This currently relies on std::atomic's implicit conversion (which is not available for all specializations / libstdc++ implementations, notably for non-standard integral types like __int128), and it prevents choosing a cheaper memory order for an emptiness check. Use an explicit atomic load instead.

#if !defined(INK_QUEUE_NT)
#define INK_ATOMICLIST_EMPTY(_x) (!(TO_PTR(FREELIST_POINTER((_x.head)))))
#else
/* ink_queue_nt.c doesn't do the FROM/TO pointer swizzling */
#define INK_ATOMICLIST_EMPTY(_x) (!((FREELIST_POINTER((_x.head)))))
#endif

[Copilot]

Pull request overview

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

[JosiahWI]

Rebased on 0847dc6

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/tscore/ink_queue.cc:199

• ink_freelist_init() currently overwrites f->alignment with ats_hugepage_size() when hugepages are enabled (lines 200-203). But f->alignment is also treated as the per-item alignment elsewhere (e.g. the new freelist_free() / freelist_bulkfree() release asserts and is_next_ptr_aligned()). For hugepage-backed freelists that pack many small items into one hugepage allocation (e.g. IOBufferData blocks), only the first item in a chunk can be hugepage-aligned, so this makes the new alignment checks fail for most items and can abort in release builds.

  // It is never useful to have alignment requirement looser than a page size
  // so clip it. This makes the item alignment checks in the actual allocator simpler.
  f->alignment         = alignment;
  f->use_hugepages     = ats_hugepage_enabled() && use_hugepages;
  f->hugepages_failure = 0;

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/tscore/ink_queue.cc:210

• In hugepage mode (use_hugepages == true), f->alignment is currently overwritten with ats_hugepage_size(). That makes the new release-assert alignment checks in freelist_free()/freelist_bulkfree() require every individual cell to be hugepage-aligned, which is impossible for typical freelists (e.g., IOBuffer freelists allocate many cells per hugepage-backed chunk). This will trip ink_release_assert(is_addr_aligned(item, f->alignment)) during chunk seeding / normal frees when hugepages are enabled.

f->alignment should remain the per-item alignment (clipped to at most ats_pagesize()), while hugepage size should only affect the chunk allocation alignment in freelist_new().

  // It is never useful to have alignment requirement looser than a page size
  // so clip it. This makes the item alignment checks in the actual allocator simpler.
  f->alignment         = alignment;
  f->use_hugepages     = ats_hugepage_enabled() && use_hugepages;
  f->hugepages_failure = 0;
  if (f->use_hugepages) {
    // for hugepages, always make the allocation alignment on a hugepage boundary
    f->alignment = ats_hugepage_size();
    f->type_size = type_size;
  } else {
    if (f->alignment > ats_pagesize()) {
      f->alignment = ats_pagesize();
    }
    // Make sure we align *all* the objects in the allocation, not just the first one
    f->type_size = INK_ALIGN(type_size, f->alignment);
  }

[phongn]

Apologies for stepping on your toes with #13571. My change is relatively constrained, mostly to get RISC-V support working; I was going to think about doing exactly what you do in this PR next.

[phongn]

Note: -mcx16 will not assume that cmpxchg16b is available, which is important for if we ever replace __sync_val_compare_and_swap with something more modern: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878

[phongn]

Thanks for this — the direction is right, and several pieces here are genuinely valuable: the std::atomic/memcpy/placement-new conversion removes real UB, the alignment assertions are sound, and the .load() conversions fix part of #13572. I ran your branch through a fairly deep review (stress tests on both head configurations, a memory-order audit, and benchmarks on the configurations your published numbers could not cover). The implementation itself held up well under stress; the problems I found are in the configuration gate and in what it silently changes. Details below, roughly in severity order.

1. The TS_HAS_128BIT_ATOMIC probe fails on every GCC build (blocker)

The probe in Check128BitAtomic.cmake compiles and links std::atomic<__int128> without -latomic. GCC never inlines 16-byte atomics, even with -mcx16 — it always emits __atomic_compare_exchange_16 library calls (GCC PR80878) — so the probe's link step fails on GCC regardless of flags. Only clang with -mcx16 passes. Verified on gcc 11.5 / clang 21:

g++     plain  : undefined reference to `__atomic_compare_exchange_16'
g++     -mcx16 : undefined reference to `__atomic_compare_exchange_16'
clang++ plain  : undefined reference to `__atomic_compare_exchange_16'
clang++ -mcx16 : LINKS

Building this branch confirms it: the gcc build gets #define TS_HAS_128BIT_ATOMIC 0, the clang build gets 1. I suspect this is also why your own "after" benchmark ran with a 64-bit head_p — it is not a hardware limitation, it is this probe.

Consequence: TS_128BIT_QUEUE is 0 on Linux x86-64 and aarch64 GCC builds — essentially every distro/production build — which silently drop from the 128-bit head (64-bit version field) to the packed head (15-bit version on x86-64, 11-bit on aarch64), with behavior now diverging by compiler.

2. The packed head's narrow version field breaks LogObject's reference counting (high, cascades from 1)

LogObject::_checkout_write increments the head version once per checkout, and on buffer swap credits m_references += FREELIST_VERSION(old_h) - 1 — the version field is a cumulative checkout count per buffer lifetime, not just an ABA tag. A default 9 MB log buffer with 100–200-byte entries absorbs roughly 45–90k checkouts before it fills. That exceeds a 15-bit version (32,768) and dwarfs an 11-bit one (2,048). A wrapped version undercounts m_references, so the flush thread can free a LogBuffer while writers still hold references.

This hazard already exists on master for the packed-tier platforms (ppc64/mips64), where nobody runs logging-heavy ATS. Because of finding 1, this PR would promote it to mainstream Linux gcc builds on x86-64 and aarch64.

3. With the head width held constant, the locks are a significant regression

Ed. note: This benchmark was run with a Debug release build and is invalid; see next comment below for a valid benchmarking run.

The PR's before/after comparison changes two variables at once (128-bit lock-free head → 64-bit packed head + locks). I benchmarked tools/benchmark/benchmark_FreeList with the head width held constant (clang builds, where TS_128BIT_QUEUE=1 on both sides), plus the gcc pairing. Ice Lake, 8 vCPUs, --benchmark-samples 30, dev preset:

Configuration	1 thread	2 threads	6 threads
master, clang (128-bit head, lock-free)	231.5 ms	661.7 ms	2.157 s
this PR, clang (128-bit head + pop locks)	365.9 ms (+58%)	901.0 ms (+36%)	3.323 s (+54%)
master, gcc (128-bit head, __sync, lock-free)	245.8 ms	759.7 ms	2.719 s
this PR, gcc (packed-64 head + pop locks)	247.0 ms (+0.5%)	756.6 ms (−0.4%)	3.048 s (+12%)

With the same 128-bit head, the locked version is 36–58% slower at every thread count on this host. Even the packed-64 configuration is neutral at 1–2 threads and ~12% slower at 6. The "significant performance improvement as number of threads increases" in the description appears to be the 128-bit→64-bit head change (finding 1), not the locks. One hypothesis for why the hybrid regime does poorly: locked pops and lock-free CAS pushes contend for the same head cache line, so pushers extend the poppers' critical sections.

4. ink_atomiclist_remove is the one mutation left unlocked (medium)

Pops and popall now serialize on l->m, but ink_atomiclist_remove still CASes the head lock-free, so a remove racing a locked pop can recreate exactly the use-after-free window this PR closes for #11640. Today this is safe only by the legacy discipline (remove callers must be the sole popper) — which the current callers happen to satisfy (NetHandler removes from the enable lists only on the owning net thread). Since remove is rare, taking the lock there too is cheap defense and makes the #11640 fix unconditional.

Smaller notes

• Chunk allocation in freelist_new now happens while holding the pop mutex, so a slow hugepage allocation (which can stall in compaction for tens of ms) blocks all poppers of that freelist. In exchange it eliminates the old racing-duplicate-chunk behavior — probably a fine trade, but worth stating in the description.
• alignment = std::lcm(alignment, alignof(InkFreeList)) conflates the alignment of the InkFreeList bookkeeping struct with the alignment (and hence padded type_size) of user objects: an 8-byte-aligned small-object freelist now gets 16-byte object stride. Allocating the struct with alignof(InkFreeList) separately from the user alignment would avoid the bloat.
• new (item) void *{} value-initializes (a dead nullptr store) before the real next-pointer store; new (item) void * avoids it. In freelist_bulkfree the placement-new also re-runs on every CAS retry.

What held up under testing

For completeness, the things I tried to break and could not: a 6-thread conservation/double-reachability stress test (each popped item is claimed via an atomic flag, so an item reachable from two places is detected) passes repeatedly on both head configurations of this branch; the memory orders are correctly paired (pop CAS acquire/acquire, free CAS release/relaxed, push seq-cst) with the one DEBUG-only dummy_forced_read race you already document; all structs that gained a std::mutex are new-allocated so constructors run; and a full tree build passes with gcc. The core implementation is solid — the issues are all in the gate and its consequences.

Suggested fix, and relation to #13571

#13571 (just opened for the riscv64 build break, #13555) adds exactly the missing piece: a probe that retries 128-bit atomics with -latomic and links it when needed (TS_HAS_128BIT_CAS_LIBATOMIC / TS_NEEDS_LIBATOMIC_FOR_CAS). If TS_128BIT_QUEUE accepted that tier, then:

• gcc/x86-64 keeps a 128-bit head: libatomic's ifunc resolves to cmpxchg16b at runtime, so it stays lock-free with a small call overhead (~2 ns/op in my microbenchmarks) instead of silently downgrading to 15-bit versions — findings 1 and 2 disappear.
• riscv64 gets a correct lock-based head, so this PR would also fix #13555.
• Your design already tolerates lock-based heads on the pop side — the mutex is there anyway.

Happy to coordinate: if #13571 lands first, this PR rebases onto it and folds TS_HAS_128BIT_CAS_LIBATOMIC into the TS_128BIT_QUEUE gate, then re-run the benchmark with head width held constant to evaluate the locks on their own merits (or make the pop lock conditional on the platform lacking lock-free atomics).

[phongn]

Following up on my earlier review with a correction, a root-cause analysis of the regression, and a prototype that removes it.

Correction to my benchmark table

My earlier numbers were built with the dev preset, which is a Debug/low-optimization build. At low optimization the std::atomic wrapper layers in this PR do not inline, which unfairly penalized this PR's code structure relative to master's macro/union code. Your Release methodology was right and my contended numbers were wrong — apologies for the noise. Re-run with --preset release, interleaved A/B/C (each binary alternated within each rep, 3 reps, 20 samples; 1-thread means reproduce within 1%):

Configuration	1 thread	6 threads
master, clang (128-bit head, lock-free)	56.7 ms	2.19 s
this PR, clang (128-bit head + pop locks)	73.1 ms (+29%)	1.60 s (−27%)
prototype, clang (128-bit head, lock-free + atomic_ref)	56.7 ms (= master)	2.02 s
master, gcc (128-bit head, __sync, lock-free)	76.7 ms	2.58 s
this PR, gcc (packed-64 head + pop locks)	47.2 ms	1.44 s
prototype, gcc (packed-64 head, lock-free + atomic_ref)	30.1 ms	1.59 s

So in Release, the locks genuinely win under heavy contention — your instinct there was correct and my earlier claim of a contended regression was an artifact. The real cost is on the uncontended fast path: +29% per op at 1 thread on the 128-bit tier.

Why this explains the end-to-end regression

I tested three mechanisms in isolation (microbenchmark with a versioned 128-bit Treiber stack):

• Mutex fixed cost, uncontended — confirmed: the lock/unlock pair adds ~30-45% per op at 1 thread, matching the table above.
• False sharing (the std::mutex and the head share a cache line: mutex occupies bytes 0-39, head sits at offset 48) — exonerated: padding them onto separate lines changes nothing measurable.
• Lock convoy under contention — exonerated: the lock is better than CAS retry storms when contended.

In production, ProxyAllocator thread caches absorb most freelist traffic and ProtectedQueue operations rarely collide, so the fleet mostly runs the uncontended path — paying the +29% tax on every op while rarely collecting the contended win. That is consistent with the 5-10% end-to-end regression you measured internally.

Prototype: same UB fix, no locks

The #11640 bug is a data race on the speculative next-pointer read — the version-tagged CAS has always correctly rejected stale values; the algorithm does not need mutual exclusion. The prototype keeps everything else in this PR (memcpy views, placement-new lifetime handling, alignment asserts, LSan annotations) and:

• removes the three lock_guards and the now-unused std::mutex members, and
• converts every next-slot access (pop's speculative read, push/free/bulkfree stores, popall's fixup walk, remove's reads/writes) to relaxed std::atomic_ref<void *>, so the race is defined behavior.

Relaxed slot accesses compile to plain loads/stores; the release/acquire edge stays on the head CAS (slot store sequenced-before the release CAS; popper's acquire on the head synchronizes-with it). Two details worth noting because they are easy to miss: the value-initializing placement-news (new (p) void *{}) were themselves plain stores into raced slots, so the prototype uses default-init (new (p) void *, no store) followed by an atomic store; and ink_atomiclist_remove's slot accesses are converted too (it still requires the documented single-popper discipline, as it always has, but its accesses are now defined).

Branch: phongn@988a139a57 (stacked on this PR's head, cherry-pick friendly)

Validation: passes a 6-thread conservation/double-reachability stress test repeatedly on both head configurations, in both Debug and Release; performance is exactly master at 1 thread (56.7 ms) and ≈ master at 6 threads on the 128-bit tier.

Honest residuals: the Debug-only races remain (dummy_forced_read, which you already documented, and DEADBEEF's plain fills). And for InkAtomicList specifically, a stale popper can touch caller-owned memory after the pop — the same invariant master has relied on for decades (ironclad for the freelist, whose chunks are never unmapped; theoretical for arbitrary atomiclist items). The locks close that fully; atomic_ref only removes the UB. If you want maximum conservatism, a hybrid is coherent: lock-free freelist (the measured hot path) + locked atomiclist pops.

Given your internal 5-10% end-to-end regression was likely the uncontended mutex tax, the prototype should recover it — it would be great to see it through your internal benchmarking. Also worth noting from the table: gcc's packed-64 lock-free configuration (30 ms) is by far the fastest, so once the LogObject version-width hazard from my earlier review is addressed, deliberately selecting the packed head could be a legitimate follow-up optimization rather than a probe accident.