From a106ab482d420b406e32c0edbd038f7580f39f59 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Fri, 15 May 2026 15:33:05 -0500 Subject: [PATCH 01/10] Convert ink_queue implementation to std::atomic --- include/tscore/ink_queue.h | 113 +++++++--- src/proxy/logging/LogObject.cc | 6 +- src/tscore/CMakeLists.txt | 10 +- src/tscore/ink_queue.cc | 279 +++++++++++++++--------- src/tscore/unit_tests/test_ink_queue.cc | 243 +++++++++++++++++++++ 5 files changed, 506 insertions(+), 145 deletions(-) create mode 100644 src/tscore/unit_tests/test_ink_queue.cc diff --git a/include/tscore/ink_queue.h b/include/tscore/ink_queue.h index a0492213df3..52e905dfbbe 100644 --- a/include/tscore/ink_queue.h +++ b/include/tscore/ink_queue.h @@ -37,6 +37,10 @@ #include "tscore/ink_defs.h" #include "tscore/ink_apidefs.h" +#include +#include +#include + /* For information on the structure of the x86_64 memory map: @@ -71,30 +75,62 @@ void ink_queue_load_64(void *dst, void *src); /* * Generic Free List Manager */ -// Warning: head_p is read and written in multiple threads without a -// lock, use INK_QUEUE_LD to read safely. -union head_p { - head_p() : data(){}; - #if (defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4) - typedef int32_t version_type; - typedef int64_t data_type; +typedef int32_t head_p_version_type; +typedef int64_t head_p_data_type; #elif TS_HAS_128BIT_CAS - typedef int64_t version_type; - typedef __int128_t data_type; +typedef int64_t head_p_version_type; +typedef __int128_t head_p_data_type; #else - using version_type = int64_t; - using data_type = int64_t; +using head_p_version_type = int64_t; +using head_p_data_type = int64_t; #endif - struct { - void *pointer; - version_type version; - } s; +// Warning: head_p is read and written in multiple threads without a +// lock, use INK_QUEUE_LD to read safely. +using head_p = head_p_data_type; + +#if ((defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4)) || TS_HAS_128BIT_CAS - data_type data; +struct head_p_view { + void *pointer; + head_p_version_type version; }; +#elif TS_HAS_128BIT_CAS + +struct head_p_view { + void *pointer; + head_p_version_type version; +}; + +#elif defined(__x86_64__) || defined(__ia64__) || defined(__powerpc64__) || defined(__mips64) + +struct head_p_view { + int vaddr : 48; + int version : 15; + int vaddr_mode : 1; +}; +#endif + +inline head_p_view +load_head(head_p const &src) +{ + head_p_view result; + static_assert(sizeof(result) == sizeof(src)); + std::memcpy(&result, &src, sizeof(result)); + return result; +} + +#include + +inline void +store_head(head_p &dest, head_p_view const src) +{ + static_assert(sizeof(dest) == sizeof(src)); + std::memcpy(&dest, &src, sizeof(dest)); +} + /* * Why is version required? One scenario is described below * Think of a list like this -> A -> C -> D @@ -119,17 +155,13 @@ union head_p { #endif #if (defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4) -#define FREELIST_POINTER(_x) (_x).s.pointer -#define FREELIST_VERSION(_x) (_x).s.version -#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) \ - (_x).s.pointer = _p; \ - (_x).s.version = _v +#define FREELIST_POINTER(_x) load_head((_x)).pointer +#define FREELIST_VERSION(_x) load_head((_x)).version +#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) store_head((_x), head_p_view{(_p), (_v)}) #elif TS_HAS_128BIT_CAS -#define FREELIST_POINTER(_x) (_x).s.pointer -#define FREELIST_VERSION(_x) (_x).s.version -#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) \ - (_x).s.pointer = _p; \ - (_x).s.version = _v +#define FREELIST_POINTER(_x) load_head((_x)).pointer +#define FREELIST_VERSION(_x) load_head((_x)).version +#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) store_head((_x), head_p_view{(_p), (_v)}) #elif defined(__x86_64__) || defined(__ia64__) || defined(__powerpc64__) || defined(__mips64) /* Layout of FREELIST_POINTER * @@ -178,13 +210,21 @@ union head_p { #endif struct _InkFreeList { - head_p head; - const char *name; - uint32_t type_size, chunk_size, used, allocated, alignment; - uint32_t allocated_base, used_base; - uint32_t hugepages_failure; - bool use_hugepages; - int advice; + std::mutex m; + std::atomic head; + const char *name; + std::atomic used; + std::atomic allocated; + + // These fields must be initialized once and not modified after + // initialization. + uint32_t type_size, chunk_size, alignment; + + std::atomic allocated_base; + std::atomic used_base; + std::atomic hugepages_failure; + bool use_hugepages; + int advice; }; using InkFreeListOps = struct ink_freelist_ops; @@ -213,9 +253,10 @@ void ink_freelists_snap_baseline(); struct InkAtomicList { InkAtomicList() {} - head_p head{}; - const char *name = nullptr; - uint32_t offset = 0; + std::mutex m; + std::atomic head{}; + const char *name = nullptr; + uint32_t offset = 0; }; #if !defined(INK_QUEUE_NT) diff --git a/src/proxy/logging/LogObject.cc b/src/proxy/logging/LogObject.cc index 03e1316a17c..336379d4e6a 100644 --- a/src/proxy/logging/LogObject.cc +++ b/src/proxy/logging/LogObject.cc @@ -324,18 +324,18 @@ increment_pointer_version(head_p *dst) do { INK_QUEUE_LD(h, *dst); SET_FREELIST_POINTER_VERSION(new_h, FREELIST_POINTER(h), FREELIST_VERSION(h) + 1); - } while (ink_atomic_cas(&dst->data, h.data, new_h.data) == false); + } while (ink_atomic_cas(dst, h, new_h) == false); return h; } static bool -write_pointer_version(head_p *dst, head_p old_h, void *ptr, head_p::version_type vers) +write_pointer_version(head_p *dst, head_p old_h, void *ptr, head_p_version_type vers) { head_p tmp_h; SET_FREELIST_POINTER_VERSION(tmp_h, ptr, vers); - return ink_atomic_cas(&dst->data, old_h.data, tmp_h.data); + return ink_atomic_cas(dst, old_h, tmp_h); } LogBuffer * diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index 7790adc87dd..7e664b47ec3 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -107,7 +107,14 @@ else() endif() target_link_libraries( - tscore PUBLIC OpenSSL::Crypto libswoc::libswoc yaml-cpp::yaml-cpp systemtap::systemtap resolv::resolv ts::tsutil + tscore + PUBLIC atomic + OpenSSL::Crypto + libswoc::libswoc + yaml-cpp::yaml-cpp + systemtap::systemtap + resolv::resolv + ts::tsutil ) if(TS_USE_POSIX_CAP) @@ -161,6 +168,7 @@ if(BUILD_TESTING) unit_tests/test_ink_inet.cc unit_tests/test_ink_memory.cc unit_tests/test_ink_string.cc + unit_tests/test_ink_queue.cc unit_tests/test_layout.cc unit_tests/test_scoped_resource.cc unit_tests/test_Version.cc diff --git a/src/tscore/ink_queue.cc b/src/tscore/ink_queue.cc index 86cab240010..bfb3b33703f 100644 --- a/src/tscore/ink_queue.cc +++ b/src/tscore/ink_queue.cc @@ -38,9 +38,13 @@ #include "tscore/ink_config.h" +#include #include -#include +#include #include +#include +#include +#include #include #include #include @@ -57,6 +61,8 @@ #include "tscore/Diags.h" #include "tscore/JeMiAllocator.h" +#include + struct ink_freelist_ops { void *(*fl_new)(InkFreeList *); void (*fl_free)(InkFreeList *, void *); @@ -92,6 +98,9 @@ void *malloc_new(InkFreeList *f); void malloc_free(InkFreeList *f, void *item); void malloc_bulkfree(InkFreeList *f, void *head, void *tail, size_t num_item); +[[maybe_unused]] static bool is_next_ptr_aligned(InkFreeList const *f, head_p const &item); +[[maybe_unused]] static bool is_addr_aligned(void const *item, std::size_t alignment); + const ink_freelist_ops malloc_ops = {malloc_new, malloc_free, malloc_bulkfree}; const ink_freelist_ops freelist_ops = {freelist_new, freelist_free, freelist_bulkfree}; const ink_freelist_ops *default_ops = &freelist_ops; @@ -137,6 +146,15 @@ void ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size, uint32_t chunk_size, uint32_t alignment, bool use_hugepages) { + // The alignment is used as a boundary for INK_ALIGN, + // which requires a power of 2 boundary. + ink_release_assert(alignment % 2 == 0); + + // Freelist nodes are head_p objects whenever they are free, which means + // our allocation has to meet alignment requirements for both head_p objects + // and the allocator type. + alignment = std::lcm(alignment, alignof(head_p)); + InkFreeList *f; ink_freelist_list *fll; @@ -152,7 +170,7 @@ ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size, uint32 f->name = name; /* quick test for power of 2 */ - ink_assert(!(alignment & (alignment - 1))); + ink_release_assert(!(alignment & (alignment - 1))); // 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; @@ -178,7 +196,10 @@ ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size, uint32 f->chunk_size = INK_ALIGN(chunk_size * f->type_size, ats_pagesize()) / f->type_size; } Dbg(dbg_ctl_freelist_init, "<%s> Chunk Size request/actual (%" PRIu32 "/%" PRIu32 ")", name, chunk_size, f->chunk_size); - SET_FREELIST_POINTER_VERSION(f->head, FROM_PTR(0), 0); + head_p empty_head; + SET_FREELIST_POINTER_VERSION(empty_head, FROM_PTR(0), 0); + ink_assert(is_next_ptr_aligned(f, empty_head)); + f->head.store(empty_head); *fl = f; } @@ -200,7 +221,13 @@ ink_freelist_create(const char *name, uint32_t type_size, uint32_t chunk_size, u return f; } -#define ADDRESS_OF_NEXT(x, offset) ((void **)((char *)x + offset)) +static head_p * +to_head_p(void *x, std::uint64_t offset) +{ + unsigned char *addr{reinterpret_cast(x) + offset}; + // ink_assert(is_addr_aligned(x, alignof(head_p))); + return reinterpret_cast(addr); +} void * ink_freelist_new(InkFreeList *f) @@ -208,7 +235,7 @@ ink_freelist_new(InkFreeList *f) void *ptr; if (likely(ptr = freelist_global_ops->fl_new(f))) { - ink_atomic_increment(reinterpret_cast(&f->used), 1); + f->used.fetch_add(1, std::memory_order_acq_rel); } return ptr; @@ -222,10 +249,13 @@ freelist_new(InkFreeList *f) { head_p item; head_p next; - int result = 0; + bool result = false; + + std::lock_guard guard{f->m}; + + item = f->head.load(); do { - INK_QUEUE_LD(item, f->head); if (TO_PTR(FREELIST_POINTER(item)) == nullptr) { uint32_t i; void *newp = nullptr; @@ -248,13 +278,14 @@ freelist_new(InkFreeList *f) if (f->advice) { ats_madvise(static_cast(newp), INK_ALIGN(alloc_size, alignment), f->advice); } - SET_FREELIST_POINTER_VERSION(item, newp, 0); + SET_FREELIST_POINTER_VERSION(item, FROM_PTR(newp), 0); + ink_assert(is_next_ptr_aligned(f, item)); - ink_atomic_increment(reinterpret_cast(&f->allocated), f->chunk_size); + f->allocated.fetch_add(f->chunk_size, std::memory_order_relaxed); /* free each of the new elements */ for (i = 0; i < f->chunk_size; i++) { - char *a = (static_cast(FREELIST_POINTER(item))) + i * f->type_size; + char *a = (static_cast(newp)) + i * f->type_size; #ifdef DEADBEEF const char str[4] = {static_cast(0xde), static_cast(0xad), static_cast(0xbe), static_cast(0xef)}; for (int j = 0; j < static_cast(f->type_size); j++) { @@ -263,10 +294,11 @@ freelist_new(InkFreeList *f) #endif freelist_free(f, a); } - } else { - SET_FREELIST_POINTER_VERSION(next, *ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), 0), FREELIST_VERSION(item) + 1); - result = ink_atomic_cas(&f->head.data, item.data, next.data); + head_p *next_ptr = reinterpret_cast(TO_PTR(FREELIST_POINTER(item))); + + SET_FREELIST_POINTER_VERSION(next, FREELIST_POINTER(*next_ptr), FREELIST_VERSION(item) + 1); + result = f->head.compare_exchange_weak(item, next, std::memory_order_acquire, std::memory_order_acquire); #ifdef SANITY if (result) { @@ -282,9 +314,10 @@ freelist_new(InkFreeList *f) } #endif /* SANITY */ } - } while (result == 0); - ink_assert(!((uintptr_t)TO_PTR(FREELIST_POINTER(item)) & (((uintptr_t)f->alignment) - 1))); + } while (result == false); + ink_assert(is_next_ptr_aligned(f, item)); + ink_assert(is_next_ptr_aligned(f, next)); return TO_PTR(FREELIST_POINTER(item)); } @@ -308,9 +341,9 @@ void ink_freelist_free(InkFreeList *f, void *item) { if (likely(item != nullptr)) { - ink_assert(f->used != 0); freelist_global_ops->fl_free(f, item); - ink_atomic_decrement(reinterpret_cast(&f->used), 1); + [[maybe_unused]] std::uint32_t old_used = f->used.fetch_sub(1, std::memory_order_acq_rel); + ink_assert(old_used != 0 && "Mismatched freelist block ref count (too many frees)"); } } @@ -320,12 +353,13 @@ namespace void freelist_free(InkFreeList *f, void *item) { - void **adr_of_next = ADDRESS_OF_NEXT(item, 0); - head_p h; - head_p item_pair; - int result = 0; + // pointer to next pointer + head_p h; + head_p item_pair; + head_p *recovered_item; + bool result = false; - // ink_assert(!((long)item&(f->alignment-1))); XXX - why is this no longer working? -bcall + ink_release_assert(is_addr_aligned(item, f->alignment)); #ifdef DEADBEEF { @@ -338,8 +372,9 @@ freelist_free(InkFreeList *f, void *item) } #endif /* DEADBEEF */ + recovered_item = new (item) head_p{}; + h = f->head.load(); while (!result) { - INK_QUEUE_LD(h, f->head); #ifdef SANITY if (TO_PTR(FREELIST_POINTER(h)) == item) { ink_abort("ink_freelist_free: trying to free item twice"); @@ -351,11 +386,16 @@ freelist_free(InkFreeList *f, void *item) dummy_forced_read(TO_PTR(FREELIST_POINTER(h))); } #endif /* SANITY */ - *adr_of_next = FREELIST_POINTER(h); + + ink_assert(is_next_ptr_aligned(f, h)); + + SET_FREELIST_POINTER_VERSION(*recovered_item, FREELIST_POINTER(h), 0); SET_FREELIST_POINTER_VERSION(item_pair, FROM_PTR(item), FREELIST_VERSION(h)); - INK_MEMORY_BARRIER; - result = ink_atomic_cas(&f->head.data, h.data, item_pair.data); + result = f->head.compare_exchange_weak(h, item_pair, std::memory_order_release, std::memory_order_relaxed); } + + ink_assert(is_next_ptr_aligned(f, h)); + ink_assert(is_next_ptr_aligned(f, *recovered_item)); } void @@ -373,10 +413,9 @@ malloc_free(InkFreeList *f, void *item) void ink_freelist_free_bulk(InkFreeList *f, void *head, void *tail, size_t num_item) { - ink_assert(f->used >= num_item); - freelist_global_ops->fl_bulkfree(f, head, tail, num_item); - ink_atomic_decrement(reinterpret_cast(&f->used), num_item); + [[maybe_unused]] std::uint32_t const old_used = f->used.fetch_sub(num_item, std::memory_order_acq_rel); + ink_assert(old_used >= num_item && "Mismatched freelist block ref count (too many frees)"); } namespace @@ -385,31 +424,32 @@ namespace void freelist_bulkfree(InkFreeList *f, void *head, void *tail, [[maybe_unused]] size_t num_item) { - void **adr_of_next = ADDRESS_OF_NEXT(tail, 0); - head_p h; - head_p item_pair; - int result = 0; + head_p h; + head_p item_pair; + head_p *recovered_tail; + bool result = false; - // ink_assert(!((long)item&(f->alignment-1))); XXX - why is this no longer working? -bcall + ink_release_assert(is_addr_aligned(head, f->alignment)); + ink_release_assert(is_addr_aligned(tail, f->alignment)); #ifdef DEADBEEF { static const char str[4] = {static_cast(0xde), static_cast(0xad), static_cast(0xbe), static_cast(0xef)}; // set the entire item to DEADBEEF; - void *temp = head; + head_p *temp = reinterpret_cast(head); for (size_t i = 0; i < num_item; i++) { for (int j = sizeof(void *); j < static_cast(f->type_size); j++) { - (static_cast(temp))[j] = str[j % 4]; + (reinterpret_cast(temp))[j] = str[j % 4]; } - *ADDRESS_OF_NEXT(temp, 0) = FROM_PTR(*ADDRESS_OF_NEXT(temp, 0)); - temp = TO_PTR(*ADDRESS_OF_NEXT(temp, 0)); + SET_FREELIST_POINTER_VERSION(*temp, FROM_PTR(FREELIST_POINTER(*temp)), FREELIST_VERSION(*temp)); + temp = to_head_p(TO_PTR(FREELIST_POINTER(*temp)), 0); } } #endif /* DEADBEEF */ + h = f->head.load(); while (!result) { - INK_QUEUE_LD(h, f->head); #ifdef SANITY if (TO_PTR(FREELIST_POINTER(h)) == head) { ink_abort("ink_freelist_free: trying to free item twice"); @@ -421,10 +461,10 @@ freelist_bulkfree(InkFreeList *f, void *head, void *tail, [[maybe_unused]] size_ dummy_forced_read(TO_PTR(FREELIST_POINTER(h))); } #endif /* SANITY */ - *adr_of_next = FREELIST_POINTER(h); + recovered_tail = new (tail) head_p{}; + SET_FREELIST_POINTER_VERSION(*recovered_tail, FREELIST_POINTER(h), 0); SET_FREELIST_POINTER_VERSION(item_pair, FROM_PTR(head), FREELIST_VERSION(h)); - INK_MEMORY_BARRIER; - result = ink_atomic_cas(&f->head.data, h.data, item_pair.data); + result = f->head.compare_exchange_weak(h, item_pair, std::memory_order_release, std::memory_order_relaxed); } } @@ -444,6 +484,18 @@ malloc_bulkfree(InkFreeList *f, void *head, void *tail, size_t num_item) } } +bool +is_next_ptr_aligned(InkFreeList const *f, head_p const &item) +{ + return is_addr_aligned(TO_PTR(FREELIST_POINTER(item)), f->alignment); +} + +bool +is_addr_aligned(void const *item, std::size_t alignment) +{ + return !(reinterpret_cast(item) & (alignment - 1u)); +} + } // end anonymous namespace void @@ -452,9 +504,9 @@ ink_freelists_snap_baseline() ink_freelist_list *fll; fll = freelists; while (fll) { - fll->fl->allocated_base = fll->fl->allocated; - fll->fl->used_base = fll->fl->used; - fll = fll->next; + fll->fl->allocated_base.store(fll->fl->allocated.load(std::memory_order_relaxed), std::memory_order_relaxed); + fll->fl->used_base.store(fll->fl->used.load(std::memory_order_relaxed), std::memory_order_relaxed); + fll = fll->next; } } @@ -472,12 +524,16 @@ ink_freelists_dump_baselinerel(FILE *f) fll = freelists; while (fll) { - int a = fll->fl->allocated - fll->fl->allocated_base; + std::uint32_t const allocated = fll->fl->allocated.load(std::memory_order_relaxed); + std::uint32_t const allocated_base = fll->fl->allocated_base.load(std::memory_order_relaxed); + int const a = allocated - allocated_base; if (a != 0) { + std::uint32_t const used = fll->fl->used.load(std::memory_order_relaxed); + std::uint32_t const used_base = fll->fl->used_base.load(std::memory_order_relaxed); fprintf(f, " %18" PRIu64 " | %18" PRIu64 " | %7u | %10u | memory/%s\n", - static_cast(fll->fl->allocated - fll->fl->allocated_base) * static_cast(fll->fl->type_size), - static_cast(fll->fl->used - fll->fl->used_base) * static_cast(fll->fl->type_size), - fll->fl->used - fll->fl->used_base, fll->fl->type_size, fll->fl->name ? fll->fl->name : ""); + static_cast(allocated - allocated_base) * static_cast(fll->fl->type_size), + static_cast(used - used_base) * static_cast(fll->fl->type_size), used - used_base, + fll->fl->type_size, fll->fl->name ? fll->fl->name : ""); } fll = fll->next; } @@ -501,13 +557,14 @@ ink_freelists_dump(FILE *f) uint64_t total_used = 0; fll = freelists; while (fll) { + std::uint64_t const allocated = fll->fl->allocated.load(std::memory_order_relaxed); + std::uint64_t const used = fll->fl->used.load(std::memory_order_relaxed); fprintf(f, " %18" PRIu64 " | %18" PRIu64 " | %18" PRIu64 " | %18" PRIu64 " | %10u | %10u | %10u | memory/%s\n", - static_cast(fll->fl->allocated) * static_cast(fll->fl->type_size), - static_cast(fll->fl->allocated), - static_cast(fll->fl->used) * static_cast(fll->fl->type_size), static_cast(fll->fl->used), - fll->fl->type_size, fll->fl->chunk_size, fll->fl->hugepages_failure, fll->fl->name ? fll->fl->name : ""); - total_allocated += static_cast(fll->fl->allocated) * static_cast(fll->fl->type_size); - total_used += static_cast(fll->fl->used) * static_cast(fll->fl->type_size); + allocated * static_cast(fll->fl->type_size), allocated, used * static_cast(fll->fl->type_size), + used, fll->fl->type_size, fll->fl->chunk_size, fll->fl->hugepages_failure.load(), + fll->fl->name ? fll->fl->name : ""); + total_allocated += allocated * static_cast(fll->fl->type_size); + total_used += used * static_cast(fll->fl->type_size); fll = fll->next; } fprintf(f, " %18" PRIu64 " | %18" PRIu64 " | | TOTAL\n", total_allocated, total_used); @@ -519,7 +576,9 @@ ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t offset_to_next) { l->name = name; l->offset = offset_to_next; - SET_FREELIST_POINTER_VERSION(l->head, FROM_PTR(0), 0); + head_p empty_head; + SET_FREELIST_POINTER_VERSION(empty_head, FROM_PTR(nullptr), 0); + l->head.store(empty_head); } void * @@ -527,20 +586,23 @@ ink_atomiclist_pop(InkAtomicList *l) { head_p item; head_p next; - int result = 0; + bool result = 0; + + std::lock_guard guard{l->m}; + + item = l->head.load(); do { - INK_QUEUE_LD(item, l->head); if (TO_PTR(FREELIST_POINTER(item)) == nullptr) { return nullptr; } - SET_FREELIST_POINTER_VERSION(next, *ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), l->offset), FREELIST_VERSION(item) + 1); - result = ink_atomic_cas(&l->head.data, item.data, next.data); - } while (result == 0); - { - void *ret = TO_PTR(FREELIST_POINTER(item)); - *ADDRESS_OF_NEXT(ret, l->offset) = nullptr; - return ret; - } + head_p *next_ptr = reinterpret_cast(reinterpret_cast(TO_PTR(FREELIST_POINTER(item))) + l->offset); + SET_FREELIST_POINTER_VERSION(next, FREELIST_POINTER(*next_ptr), FREELIST_VERSION(item) + 1); + result = l->head.compare_exchange_weak(item, next); + } while (result == false); + + head_p *ret = reinterpret_cast(reinterpret_cast(TO_PTR(FREELIST_POINTER(item))) + l->offset); + SET_FREELIST_POINTER_VERSION(*ret, nullptr, 0); + return ret; } void * @@ -548,45 +610,53 @@ ink_atomiclist_popall(InkAtomicList *l) { head_p item; head_p next; - int result = 0; + bool result = false; + + item = l->head.load(); do { - INK_QUEUE_LD(item, l->head); if (TO_PTR(FREELIST_POINTER(item)) == nullptr) { return nullptr; } SET_FREELIST_POINTER_VERSION(next, FROM_PTR(nullptr), FREELIST_VERSION(item) + 1); - result = ink_atomic_cas(&l->head.data, item.data, next.data); - } while (result == 0); - { - void *ret = TO_PTR(FREELIST_POINTER(item)); - void *e = ret; - /* fixup forward pointers */ - while (e) { - void *n = TO_PTR(*ADDRESS_OF_NEXT(e, l->offset)); - *ADDRESS_OF_NEXT(e, l->offset) = n; - e = n; - } - return ret; + result = l->head.compare_exchange_weak(item, next); + } while (result == false); + + void *ret = TO_PTR(FREELIST_POINTER(item)); + void *e = ret; + /* fixup forward pointers */ + while (e) { + head_p *e_ = to_head_p(e, l->offset); + void *n = TO_PTR(FREELIST_POINTER(*e_)); + SET_FREELIST_POINTER_VERSION(*e_, n, FREELIST_VERSION(*e_)); + e = n; } + + // ink_assert(is_addr_aligned(reinterpret_cast(ret) + l->offset, alignof(head_p))); + + return ret; } void * ink_atomiclist_push(InkAtomicList *l, void *item) { - void **adr_of_next = ADDRESS_OF_NEXT(item, l->offset); - head_p head; - head_p item_pair; - int result = 0; - void *h = nullptr; + // ink_release_assert(is_addr_aligned(reinterpret_cast(item) + l->offset, alignof(head_p))); + + head_p head; + head_p item_pair; + head_p *recovered_item; + bool result = false; + void *h = nullptr; + + head = l->head.load(); do { - INK_QUEUE_LD(head, l->head); - h = FREELIST_POINTER(head); - *adr_of_next = h; + h = FREELIST_POINTER(head); ink_assert(item != TO_PTR(h)); + + recovered_item = new (reinterpret_cast(item) + l->offset) head_p{}; + SET_FREELIST_POINTER_VERSION(*recovered_item, FREELIST_POINTER(head), 0); SET_FREELIST_POINTER_VERSION(item_pair, FROM_PTR(item), FREELIST_VERSION(head)); - INK_MEMORY_BARRIER; - result = ink_atomic_cas(&l->head.data, head.data, item_pair.data); - } while (result == 0); + result = l->head.compare_exchange_weak(head, item_pair); + } while (result == false); return TO_PTR(h); } @@ -594,26 +664,25 @@ ink_atomiclist_push(InkAtomicList *l, void *item) void * ink_atomiclist_remove(InkAtomicList *l, void *item) { - head_p head; - void *prev = nullptr; - void **addr_next = ADDRESS_OF_NEXT(item, l->offset); - void *item_next = *addr_next; - int result = 0; + head_p head; + void *prev = nullptr; + head_p *addr_next = to_head_p(item, l->offset); + void *item_next = FREELIST_POINTER(*addr_next); + bool result = 0; /* * first, try to pop it if it is first */ - INK_QUEUE_LD(head, l->head); + head = l->head.load(); while (TO_PTR(FREELIST_POINTER(head)) == item) { head_p next; SET_FREELIST_POINTER_VERSION(next, item_next, FREELIST_VERSION(head) + 1); - result = ink_atomic_cas(&l->head.data, head.data, next.data); + result = l->head.compare_exchange_weak(head, next); if (result) { - *addr_next = nullptr; + SET_FREELIST_POINTER_VERSION(*addr_next, nullptr, 0); return item; } - INK_QUEUE_LD(head, l->head); } /* @@ -621,13 +690,13 @@ ink_atomiclist_remove(InkAtomicList *l, void *item) */ prev = TO_PTR(FREELIST_POINTER(head)); while (prev) { - void **prev_adr_of_next = ADDRESS_OF_NEXT(prev, l->offset); - void *prev_prev = prev; - prev = TO_PTR(*prev_adr_of_next); + head_p *prev_adr_of_next = to_head_p(prev, l->offset); + void *prev_prev = prev; + prev = TO_PTR(FREELIST_POINTER(*prev_adr_of_next)); if (prev == item) { ink_assert(prev_prev != item_next); - *prev_adr_of_next = item_next; - *addr_next = nullptr; + SET_FREELIST_POINTER_VERSION(*prev_adr_of_next, item_next, 0); + SET_FREELIST_POINTER_VERSION(*addr_next, nullptr, 0); return item; } } diff --git a/src/tscore/unit_tests/test_ink_queue.cc b/src/tscore/unit_tests/test_ink_queue.cc new file mode 100644 index 00000000000..3da79f2512f --- /dev/null +++ b/src/tscore/unit_tests/test_ink_queue.cc @@ -0,0 +1,243 @@ +/* + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#define CATCH_CONFIG_ENABLE_BENCHMARKING +#include + +#include + +#include +#include +#include + +TEST_CASE("Freelist", "[freelist]") +{ + // There is no error reporting for this routine. The allocation + // is assumed to never fail. + InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 1, alignof(std::int32_t))}; + + SECTION("Freelist allocates aligned pointers") + { + InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 1, alignof(std::int32_t))}; + void *addr{ink_freelist_new(f)}; + + CHECK(!(reinterpret_cast(addr) & (alignof(std::int32_t) - 1))); + + ink_freelist_free(f, addr); + } + + SECTION("Two new freelist allocations") + { + std::int32_t *a{new (ink_freelist_new(f)) std::int32_t{1}}; + std::int32_t *b{new (ink_freelist_new(f)) std::int32_t{2}}; + + CHECK(((*a == 1) && (*b == 2))); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } + + SECTION("Freelist stack behavior") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + + void *x{ink_freelist_new(f)}; + void *y{ink_freelist_new(f)}; + + CHECK((a == y && b == x)); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } +} + +TEST_CASE("Empty atomic list", "[atomiclist]") +{ + InkAtomicList l; + ink_atomiclist_init(&l, "test#1", 0); + + CHECK(nullptr == ink_atomiclist_pop(&l)); +} + +TEST_CASE("Popall from empty atomic list", "[atomiclist]") +{ + InkAtomicList l; + ink_atomiclist_init(&l, "test#1", 0); + + CHECK(nullptr == ink_atomiclist_popall(&l)); +} + +TEST_CASE("Atomic list", "[atomiclist]") +{ + InkAtomicList l; + ink_atomiclist_init(&l, "test#1", 0); + + // We allocate memory this way to ensure proper alignment for atomiclist. + InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 1, alignof(std::int32_t))}; + + SECTION("Atomic list becomes empty after push and pop") + { + ink_atomiclist_push(&l, ink_freelist_new(f)); + void *a{ink_atomiclist_pop(&l)}; + + CHECK(nullptr == ink_atomiclist_pop(&l)); + + ink_freelist_free(f, a); + } + + SECTION("Atomic list stack behavior") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_atomiclist_push(&l, a); + ink_atomiclist_push(&l, b); + + void *x{ink_atomiclist_pop(&l)}; + void *y{ink_atomiclist_pop(&l)}; + + CHECK((a == y && b == x)); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } + + SECTION("Atomic list remove head") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_atomiclist_push(&l, a); + ink_atomiclist_push(&l, b); + + CHECK(b == ink_atomiclist_remove(&l, b)); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } + + SECTION("Atomic list remove tail") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_atomiclist_push(&l, a); + ink_atomiclist_push(&l, b); + + CHECK(a == ink_atomiclist_remove(&l, a)); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } + + SECTION("Atomic list becomes empty after popall") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_atomiclist_push(&l, a); + ink_atomiclist_push(&l, b); + + ink_atomiclist_popall(&l); + CHECK(nullptr == ink_atomiclist_popall(&l)); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } + + SECTION("Atomic list popall behavior") + { + void *a{ink_freelist_new(f)}; + void *b{ink_freelist_new(f)}; + + ink_atomiclist_push(&l, a); + ink_atomiclist_push(&l, b); + + head_p *head{reinterpret_cast(ink_atomiclist_popall(&l))}; + head_p *tail{reinterpret_cast(FREELIST_POINTER(*head))}; + + CHECK(((head == b) && (tail == a))); + + ink_freelist_free(f, a); + ink_freelist_free(f, b); + } +} + +TEST_CASE("Freelist benchmarks", "[freelist][bench]") +{ + BENCHMARK_ADVANCED("Single threaded alloc")(Catch::Benchmark::Chronometer meter) + { + InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 4, alignof(std::int32_t))}; + + ink_freelist_new(f); + + meter.measure([&]() { return ink_freelist_new(f); }); + }; + + BENCHMARK_ADVANCED("Single threaded free")(Catch::Benchmark::Chronometer meter) + { + InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 4, alignof(std::int32_t))}; + + std::vector ptrs; + ptrs.resize(meter.runs()); + for (auto &x : ptrs) { + x = ink_freelist_new(f); + } + + meter.measure([&](int i) { return ink_freelist_free(f, ptrs[i]); }); + }; + + /* + BENCHMARK_ADVANCED("yay")(Catch::Benchmark::Chronometer meter) + { + InkFreeList *f{ink_freelist_create("yay", 4, 8, 16)}; + + std::atomic done(false); + + auto const use_memory{[&]() { + void *storage{ink_freelist_new(f)}; + std::int32_t *n{new (storage) std::int32_t{}}; + *n = 10; + ink_freelist_free(f, n); + }}; + + auto const distract{[&]() { + while (!done) { + use_memory(); + } + }}; + + std::jthread contender{distract}; + + meter.measure([&]() { + for (int i{0}; i < 10; ++i) { + use_memory(); + } + }); + + done = true; + }; + */ +} From 6a52eb11e06f7bf4ea3dac79956a40bb6a1a1e31 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 06:04:32 -0500 Subject: [PATCH 02/10] Make changes suggested by Copilot * Fix macro definitions for all platforms The definitions still used the `data` member of the old `head_p` struct. * Clean up `head_p_view` definitions * Remove unused `` include * Make `freelist_init` alignment check consistent * Test atomiclist with offset * Add @file description to test_ink_queue.cc * Add missing `` include to unit tests * Construct `InkFreeList` with placement new --- include/tscore/ink_queue.h | 39 ++++-------- src/tscore/ink_queue.cc | 18 +++--- src/tscore/unit_tests/test_ink_queue.cc | 80 +++++++++---------------- 3 files changed, 51 insertions(+), 86 deletions(-) diff --git a/include/tscore/ink_queue.h b/include/tscore/ink_queue.h index 52e905dfbbe..34fda9ee20e 100644 --- a/include/tscore/ink_queue.h +++ b/include/tscore/ink_queue.h @@ -92,25 +92,14 @@ using head_p = head_p_data_type; #if ((defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4)) || TS_HAS_128BIT_CAS +// This struct maps to head_p on the above platforms. On other platforms, +// bitshifting is used to read head_p. +// See FREELIST_POINTER for the layout on those platforms. struct head_p_view { void *pointer; head_p_version_type version; }; -#elif TS_HAS_128BIT_CAS - -struct head_p_view { - void *pointer; - head_p_version_type version; -}; - -#elif defined(__x86_64__) || defined(__ia64__) || defined(__powerpc64__) || defined(__mips64) - -struct head_p_view { - int vaddr : 48; - int version : 15; - int vaddr_mode : 1; -}; #endif inline head_p_view @@ -122,8 +111,6 @@ load_head(head_p const &src) return result; } -#include - inline void store_head(head_p &dest, head_p_view const src) { @@ -176,16 +163,14 @@ store_head(head_p &dest, head_p_view const src) */ #if ((~0 >> 1) < 0) /* the shift is `arithmetic' */ -#define FREELIST_POINTER(_x) \ - ((void *)((((intptr_t)(_x).data) & 0x0000FFFFFFFFFFFFLL) | ((((intptr_t)(_x).data) >> 63) << 48))) // sign extend +#define FREELIST_POINTER(_x) ((void *)((((intptr_t)(_x)) & 0x0000FFFFFFFFFFFFLL) | ((((intptr_t)(_x)) >> 63) << 48))) // sign extend #else /* the shift is `logical' */ -#define FREELIST_POINTER(_x) \ - ((void *)((((intptr_t)(_x).data) & 0x0000FFFFFFFFFFFFLL) | ((~((((intptr_t)(_x).data) >> 63) - 1)) << 48))) +#define FREELIST_POINTER(_x) ((void *)((((intptr_t)(_x)) & 0x0000FFFFFFFFFFFFLL) | ((~((((intptr_t)(_x)) >> 63) - 1)) << 48))) #endif -#define FREELIST_VERSION(_x) ((((intptr_t)(_x).data) & 0x7FFF000000000000LL) >> 48) -#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) (_x).data = ((((intptr_t)(_p)) & 0x8000FFFFFFFFFFFFLL) | (((_v) & 0x7FFFLL) << 48)) +#define FREELIST_VERSION(_x) ((((intptr_t)(_x)) & 0x7FFF000000000000LL) >> 48) +#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) (_x) = ((((intptr_t)(_p)) & 0x8000FFFFFFFFFFFFLL) | (((_v) & 0x7FFFLL) << 48)) #elif defined(__aarch64__) /* Layout of FREELIST_POINTER * @@ -195,16 +180,14 @@ store_head(head_p &dest, head_p_view const src) */ #if ((~0 >> 1) < 0) /* the shift is `arithmetic' */ -#define FREELIST_POINTER(_x) \ - ((void *)((((intptr_t)(_x).data) & 0x000FFFFFFFFFFFFFLL) | ((((intptr_t)(_x).data) >> 63) << 52))) // sign extend +#define FREELIST_POINTER(_x) ((void *)((((intptr_t)(_x)) & 0x000FFFFFFFFFFFFFLL) | ((((intptr_t)(_x)) >> 63) << 52))) // sign extend #else /* the shift is `logical' */ -#define FREELIST_POINTER(_x) \ - ((void *)((((intptr_t)(_x).data) & 0x000FFFFFFFFFFFFFLL) | ((~((((intptr_t)(_x).data) >> 63) - 1)) << 52))) +#define FREELIST_POINTER(_x) ((void *)((((intptr_t)(_x)) & 0x000FFFFFFFFFFFFFLL) | ((~((((intptr_t)(_x)) >> 63) - 1)) << 52))) #endif -#define FREELIST_VERSION(_x) ((((intptr_t)(_x).data) & 0x7FF0000000000000LL) >> 52) -#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) (_x).data = ((((intptr_t)(_p)) & 0x800FFFFFFFFFFFFFLL) | (((_v) & 0x7FFLL) << 52)) +#define FREELIST_VERSION(_x) ((((intptr_t)(_x)) & 0x7FF0000000000000LL) >> 52) +#define SET_FREELIST_POINTER_VERSION(_x, _p, _v) (_x) = ((((intptr_t)(_p)) & 0x800FFFFFFFFFFFFFLL) | (((_v) & 0x7FFLL) << 52)) #else #error "unsupported processor" #endif diff --git a/src/tscore/ink_queue.cc b/src/tscore/ink_queue.cc index bfb3b33703f..e5c1cb34099 100644 --- a/src/tscore/ink_queue.cc +++ b/src/tscore/ink_queue.cc @@ -61,8 +61,6 @@ #include "tscore/Diags.h" #include "tscore/JeMiAllocator.h" -#include - struct ink_freelist_ops { void *(*fl_new)(InkFreeList *); void (*fl_free)(InkFreeList *, void *); @@ -148,7 +146,7 @@ ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size, uint32 { // The alignment is used as a boundary for INK_ALIGN, // which requires a power of 2 boundary. - ink_release_assert(alignment % 2 == 0); + ink_release_assert(alignment != 0 && (alignment & (alignment - 1u)) == 0); // Freelist nodes are head_p objects whenever they are free, which means // our allocation has to meet alignment requirements for both head_p objects @@ -160,8 +158,13 @@ ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size, uint32 /* its safe to add to this global list because ink_freelist_init() is only called from single-threaded initialization code. */ - f = static_cast(ats_memalign(alignment, sizeof(InkFreeList))); - ink_zero(*f); + f = new (ats_memalign(alignment, sizeof(InkFreeList))) InkFreeList; + f->used = 0; + f->allocated = 0; + f->allocated_base = 0; + f->used_base = 0; + f->hugepages_failure = 0; + f->advice = 0; fll = static_cast(ats_malloc(sizeof(ink_freelist_list))); fll->fl = f; @@ -600,8 +603,9 @@ ink_atomiclist_pop(InkAtomicList *l) result = l->head.compare_exchange_weak(item, next); } while (result == false); - head_p *ret = reinterpret_cast(reinterpret_cast(TO_PTR(FREELIST_POINTER(item))) + l->offset); - SET_FREELIST_POINTER_VERSION(*ret, nullptr, 0); + void *ret = TO_PTR(FREELIST_POINTER(item)); + head_p *ret_ = reinterpret_cast(reinterpret_cast(ret) + l->offset); + SET_FREELIST_POINTER_VERSION(*ret_, nullptr, 0); return ret; } diff --git a/src/tscore/unit_tests/test_ink_queue.cc b/src/tscore/unit_tests/test_ink_queue.cc index 3da79f2512f..6910ced35e4 100644 --- a/src/tscore/unit_tests/test_ink_queue.cc +++ b/src/tscore/unit_tests/test_ink_queue.cc @@ -1,22 +1,24 @@ -/* +/** @file - @section license License + Freelist and Atomiclist tests - Licensed to the Apache Software Foundation (ASF) under one - or more contributor license agreements. See the NOTICE file - distributed with this work for additional information - regarding copyright ownership. The ASF licenses this file - to you under the Apache License, Version 2.0 (the - "License"); you may not use this file except in compliance - with the License. You may obtain a copy of the License at + @section license License - http://www.apache.org/licenses/LICENSE-2.0 + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. */ #define CATCH_CONFIG_ENABLE_BENCHMARKING @@ -25,8 +27,10 @@ #include #include +#include #include #include +#include TEST_CASE("Freelist", "[freelist]") { @@ -91,11 +95,16 @@ TEST_CASE("Popall from empty atomic list", "[atomiclist]") TEST_CASE("Atomic list", "[atomiclist]") { + struct test_type { + std::int32_t i; + head_p next; + }; + InkAtomicList l; - ink_atomiclist_init(&l, "test#1", 0); + ink_atomiclist_init(&l, "test#1", offsetof(test_type, next)); // We allocate memory this way to ensure proper alignment for atomiclist. - InkFreeList *f{ink_freelist_create("test#1", sizeof(std::int32_t), 1, alignof(std::int32_t))}; + InkFreeList *f{ink_freelist_create("test#1", sizeof(test_type), 1, alignof(test_type))}; SECTION("Atomic list becomes empty after push and pop") { @@ -175,8 +184,9 @@ TEST_CASE("Atomic list", "[atomiclist]") ink_atomiclist_push(&l, a); ink_atomiclist_push(&l, b); - head_p *head{reinterpret_cast(ink_atomiclist_popall(&l))}; - head_p *tail{reinterpret_cast(FREELIST_POINTER(*head))}; + void *head{ink_atomiclist_popall(&l)}; + head_p *head_{reinterpret_cast(reinterpret_cast(head) + l.offset)}; + void *tail{FREELIST_POINTER(*head_)}; CHECK(((head == b) && (tail == a))); @@ -208,36 +218,4 @@ TEST_CASE("Freelist benchmarks", "[freelist][bench]") meter.measure([&](int i) { return ink_freelist_free(f, ptrs[i]); }); }; - - /* - BENCHMARK_ADVANCED("yay")(Catch::Benchmark::Chronometer meter) - { - InkFreeList *f{ink_freelist_create("yay", 4, 8, 16)}; - - std::atomic done(false); - - auto const use_memory{[&]() { - void *storage{ink_freelist_new(f)}; - std::int32_t *n{new (storage) std::int32_t{}}; - *n = 10; - ink_freelist_free(f, n); - }}; - - auto const distract{[&]() { - while (!done) { - use_memory(); - } - }}; - - std::jthread contender{distract}; - - meter.measure([&]() { - for (int i{0}; i < 10; ++i) { - use_memory(); - } - }); - - done = true; - }; - */ } From 5eb6d1d515b88b800c3e856e2b6291c0dbc8d9c3 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 07:22:53 -0500 Subject: [PATCH 03/10] Make changes suggested by Copilot * Only link libatomic when using 128bit atomics and lib exists --- src/tscore/CMakeLists.txt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index 7e664b47ec3..f54cf384ead 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -106,15 +106,15 @@ else() target_sources(tscore PRIVATE HKDF_openssl.cc) endif() +if(TS_HAS_128BIT_CAS) + find_library(atomic QUIET) + if(atomic_FOUND) + target_link_libraries(tscore PUBLIC atomic) + endif() +endif() + target_link_libraries( - tscore - PUBLIC atomic - OpenSSL::Crypto - libswoc::libswoc - yaml-cpp::yaml-cpp - systemtap::systemtap - resolv::resolv - ts::tsutil + tscore PUBLIC OpenSSL::Crypto libswoc::libswoc yaml-cpp::yaml-cpp systemtap::systemtap resolv::resolv ts::tsutil ) if(TS_USE_POSIX_CAP) From a508b042b13018c070c1d340b0f49bc91a12ccc7 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 07:50:50 -0500 Subject: [PATCH 04/10] Fix race condition with assertion --- src/tscore/ink_queue.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/tscore/ink_queue.cc b/src/tscore/ink_queue.cc index e5c1cb34099..4aac9e2d71c 100644 --- a/src/tscore/ink_queue.cc +++ b/src/tscore/ink_queue.cc @@ -390,15 +390,16 @@ freelist_free(InkFreeList *f, void *item) } #endif /* SANITY */ - ink_assert(is_next_ptr_aligned(f, h)); - SET_FREELIST_POINTER_VERSION(*recovered_item, FREELIST_POINTER(h), 0); SET_FREELIST_POINTER_VERSION(item_pair, FROM_PTR(item), FREELIST_VERSION(h)); + + // This assertion has to happen-before the node is in the list and + // can be handed out to an allocator. + ink_assert(is_next_ptr_aligned(f, *recovered_item)); result = f->head.compare_exchange_weak(h, item_pair, std::memory_order_release, std::memory_order_relaxed); } ink_assert(is_next_ptr_aligned(f, h)); - ink_assert(is_next_ptr_aligned(f, *recovered_item)); } void From 6f285b3d2eccd09a97464100005d28b02f53b8bb Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 07:54:55 -0500 Subject: [PATCH 05/10] Link libatomic on all platforms but Apple --- src/tscore/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index f54cf384ead..e7e9cbe8b7a 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -108,7 +108,7 @@ endif() if(TS_HAS_128BIT_CAS) find_library(atomic QUIET) - if(atomic_FOUND) + if(APPLE) target_link_libraries(tscore PUBLIC atomic) endif() endif() From bdd3c5d7c0bc6e8f57c9729351471c9c5e48769b Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 08:10:10 -0500 Subject: [PATCH 06/10] Fix incorrect check for non-Apple platforms --- src/tscore/CMakeLists.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index e7e9cbe8b7a..17771d77322 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -107,8 +107,7 @@ else() endif() if(TS_HAS_128BIT_CAS) - find_library(atomic QUIET) - if(APPLE) + if(NOT APPLE) target_link_libraries(tscore PUBLIC atomic) endif() endif() From 5d6ed37973ad33bb64679c09f59d4a040fadfc65 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 08:38:35 -0500 Subject: [PATCH 07/10] Implement complete cross-platform libatomic check --- src/tscore/CMakeLists.txt | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index 17771d77322..c25b560fabb 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -107,7 +107,27 @@ else() endif() if(TS_HAS_128BIT_CAS) - if(NOT APPLE) + set(ATOMICLIB_TEST_PROGRAM + " + #include + + std::atomic<__int128> g_atomic; + + int main() { } + " + ) + + check_cxx_source_compiles("${ATOMICLIB_TEST_PROGRAM}" NEED_LIBATOMIC) + + if(NEED_LIBATOMIC) + set(CMAKE_REQUIRED_LIBRARIES atomic) + check_cxx_source_compiles("${ATOMICLIB_TEST_PROGRAM}" HAVE_LIBATOMIC) + unset(CMAKE_REQUIRED_LIBRARIES) + + if(NOT HAVE_LIBATOMIC) + message(FATAL_ERROR "Cannot build 128 bit atomics") + endif() + target_link_libraries(tscore PUBLIC atomic) endif() endif() From 7254197163d9c0336bc4a7e76a99cf1cf0e29e35 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 09:07:36 -0500 Subject: [PATCH 08/10] Disable std::atomic<__int128> on lacking platforms --- src/tscore/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index c25b560fabb..18d96e4d1d3 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -125,7 +125,8 @@ if(TS_HAS_128BIT_CAS) unset(CMAKE_REQUIRED_LIBRARIES) if(NOT HAVE_LIBATOMIC) - message(FATAL_ERROR "Cannot build 128 bit atomics") + message(WARNING "Cannot build 128 bit atomics") + set(TS_HAS_128BIT_CAS FALSE) endif() target_link_libraries(tscore PUBLIC atomic) From be6996e435cc3d00bf524ce0219fa77e1df05355 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 09:27:10 -0500 Subject: [PATCH 09/10] Do not link libatomic when not available --- src/tscore/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index 18d96e4d1d3..4a8c98dd2b8 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -127,9 +127,10 @@ if(TS_HAS_128BIT_CAS) if(NOT HAVE_LIBATOMIC) message(WARNING "Cannot build 128 bit atomics") set(TS_HAS_128BIT_CAS FALSE) + else() + target_link_libraries(tscore PUBLIC atomic) endif() - target_link_libraries(tscore PUBLIC atomic) endif() endif() From 020b82ccfc9977925edbf6089515c9cb92abdbd1 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 19 May 2026 11:14:28 -0500 Subject: [PATCH 10/10] Base `TS_HAS_128BIT_CAS` on `__atomic` hardware --- cmake/Check128BitCas.cmake | 10 ++++++---- include/tscore/ink_queue.h | 4 ++-- src/tscore/CMakeLists.txt | 28 ---------------------------- 3 files changed, 8 insertions(+), 34 deletions(-) diff --git a/cmake/Check128BitCas.cmake b/cmake/Check128BitCas.cmake index 850bc398fe3..7c39d35dae1 100644 --- a/cmake/Check128BitCas.cmake +++ b/cmake/Check128BitCas.cmake @@ -25,16 +25,18 @@ set(CHECK_PROGRAM " - int main(void) + #include + + int main() { - __int128_t x = 0; - return __sync_bool_compare_and_swap(&x,0,10); + std::atomic<__int128> x{0}; + return x.compare_exchange_strong(10, 0); } " ) include(CheckCSourceCompiles) -check_c_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS) +check_cxx_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS) if(NOT TS_HAS_128BIT_CAS) unset(TS_HAS_128BIT_CAS CACHE) diff --git a/include/tscore/ink_queue.h b/include/tscore/ink_queue.h index 34fda9ee20e..dda25d5245f 100644 --- a/include/tscore/ink_queue.h +++ b/include/tscore/ink_queue.h @@ -100,8 +100,6 @@ struct head_p_view { head_p_version_type version; }; -#endif - inline head_p_view load_head(head_p const &src) { @@ -118,6 +116,8 @@ store_head(head_p &dest, head_p_view const src) std::memcpy(&dest, &src, sizeof(dest)); } +#endif + /* * Why is version required? One scenario is described below * Think of a list like this -> A -> C -> D diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index 4a8c98dd2b8..a1a99b706ce 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -106,34 +106,6 @@ else() target_sources(tscore PRIVATE HKDF_openssl.cc) endif() -if(TS_HAS_128BIT_CAS) - set(ATOMICLIB_TEST_PROGRAM - " - #include - - std::atomic<__int128> g_atomic; - - int main() { } - " - ) - - check_cxx_source_compiles("${ATOMICLIB_TEST_PROGRAM}" NEED_LIBATOMIC) - - if(NEED_LIBATOMIC) - set(CMAKE_REQUIRED_LIBRARIES atomic) - check_cxx_source_compiles("${ATOMICLIB_TEST_PROGRAM}" HAVE_LIBATOMIC) - unset(CMAKE_REQUIRED_LIBRARIES) - - if(NOT HAVE_LIBATOMIC) - message(WARNING "Cannot build 128 bit atomics") - set(TS_HAS_128BIT_CAS FALSE) - else() - target_link_libraries(tscore PUBLIC atomic) - endif() - - endif() -endif() - target_link_libraries( tscore PUBLIC OpenSSL::Crypto libswoc::libswoc yaml-cpp::yaml-cpp systemtap::systemtap resolv::resolv ts::tsutil )