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)

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.

[JosiahWI]

Nice catch. I believe than analysis is correct.

[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);
  }

[JosiahWI]

[approve ci freebsd]