Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3262482786
##########
include/tscore/ink_queue.h:
##########
@@ -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
Review Comment:
`head_p` is now an alias to the underlying integral `head_p_data_type`, but
some existing platform-specific `FREELIST_*` macros (e.g. the 64-bit
tagged-pointer layout when `TS_HAS_128BIT_CAS` is false) still expect `head_p`
to have a `.data` member. With `head_p` no longer being a struct/union, those
macros will no longer compile / will misbehave. Please update the remaining
`FREELIST_POINTER` / `FREELIST_VERSION` / `SET_FREELIST_POINTER_VERSION`
definitions to match the new `head_p` representation across all platform
branches.
##########
include/tscore/ink_queue.h:
##########
@@ -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;
+};
+
Review Comment:
The `head_p_view` definition has a redundant / unreachable `#elif
TS_HAS_128BIT_CAS` branch: the preceding `#if ... || TS_HAS_128BIT_CAS` already
covers that case. This makes the preprocessor logic harder to reason about and
suggests an accidental duplication—please simplify the conditional chain so
each platform/configuration is represented exactly once.
##########
src/tscore/ink_queue.cc:
##########
@@ -57,6 +61,8 @@
#include "tscore/Diags.h"
#include "tscore/JeMiAllocator.h"
+#include <iostream>
+
Review Comment:
`#include <iostream>` is added here but not used anywhere in this file.
Please remove it to avoid unnecessary compile-time overhead and extra
dependencies in `tscore`.
##########
src/tscore/ink_queue.cc:
##########
@@ -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);
Review Comment:
`alignment % 2 == 0` rejects `alignment == 1`, but 1 is a valid power-of-two
alignment (and the API comment says only that alignment must be a power of 2).
If a caller requests byte alignment, this will now abort in release builds.
Consider replacing this with a direct power-of-two check (e.g., `alignment != 0
&& (alignment & (alignment - 1)) == 0`) before using it with `INK_ALIGN`.
##########
include/tscore/ink_queue.h:
##########
@@ -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 <iostream>
+
Review Comment:
`#include <iostream>` was added in this header but isn't used here. Pulling
iostream into a widely included header increases compile times and can create
unwanted dependencies; please remove it (and keep streaming includes limited to
translation units that actually need them).
##########
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
Review Comment:
This new unit test file’s header doesn’t follow the usual ATS file header
format (e.g., `/** @file` and a short description before the license block, as
seen in other unit tests). Please align it with the standard header template
used throughout the repo for new source/test files.
##########
src/tscore/ink_queue.cc:
##########
@@ -519,115 +576,127 @@ 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 *
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<head_p *>(reinterpret_cast<unsigned
char *>(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<head_p *>(reinterpret_cast<unsigned char
*>(TO_PTR(FREELIST_POINTER(item))) + l->offset);
+ SET_FREELIST_POINTER_VERSION(*ret, nullptr, 0);
+ return ret;
}
void *
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<unsigned char *>(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<unsigned char
*>(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<unsigned char *>(item) + l->offset)
head_p{};
+ SET_FREELIST_POINTER_VERSION(*recovered_item, FREELIST_POINTER(head), 0);
Review Comment:
`ink_atomiclist_push()` constructs and writes a `head_p` object at `item +
l->offset` via placement-new. For many existing call sites `offset` points to a
plain `C* next` field (e.g., `Event::link.next`), which is pointer-sized and
only pointer-aligned. On `TS_HAS_128BIT_CAS` builds `head_p` is 16 bytes and
16-byte aligned, so this will (1) overwrite past the end of the `next` field
(memory corruption) and (2) potentially perform misaligned 16-byte accesses
(UB). The atomic list implementation needs to continue treating the per-node
'next' storage as a pointer-sized field (use memcpy-based load/store of `void*`
if you want to avoid type-punning), rather than storing a full `head_p` there.
##########
src/tscore/ink_queue.cc:
##########
@@ -519,115 +576,127 @@ 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 *
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<head_p *>(reinterpret_cast<unsigned
char *>(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<head_p *>(reinterpret_cast<unsigned char
*>(TO_PTR(FREELIST_POINTER(item))) + l->offset);
+ SET_FREELIST_POINTER_VERSION(*ret, nullptr, 0);
Review Comment:
`ink_atomiclist_pop()` should return the popped item pointer, but it
currently returns a pointer to the link field at `item + offset` (`head_p *ret
= ...; return ret;`). This breaks callers that use a non-zero offset (e.g.,
`ProtectedQueue` uses `Event::link.next`), and will return an invalid pointer
type/value to the caller.
##########
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 <catch2/catch_all.hpp>
+
+#include <tscore/ink_queue.h>
+
+#include <atomic>
+#include <cstdint>
+#include <thread>
+
+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<std::uintptr_t>(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<head_p *>(ink_atomiclist_popall(&l))};
+ head_p *tail{reinterpret_cast<head_p *>(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<void *> ptrs;
+ ptrs.resize(meter.runs());
+ for (auto &x : ptrs) {
Review Comment:
`std::vector` is used in the benchmark sections but `<vector>` isn’t
included in this file, which will fail compilation on standard-compliant
toolchains. Please add the missing include.
##########
include/tscore/ink_queue.h:
##########
@@ -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_p> head;
+ const char *name;
+ std::atomic<std::uint32_t> used;
+ std::atomic<std::uint32_t> allocated;
+
Review Comment:
`InkFreeList` now has non-trivial members (`std::mutex`, `std::atomic`), but
the allocator path in `ink_freelist_init()` still treats this as a
trivially-zeroable POD (it uses `ink_zero(*f)`, i.e. memset). Memsetting
`std::mutex` / `std::atomic` is undefined behavior. With these members present,
`InkFreeList` must be properly constructed (e.g., placement-new) and its fields
explicitly initialized, rather than zeroed wholesale.
##########
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
Review Comment:
Linking `atomic` unconditionally can break toolchains/platforms that don't
ship a separate `libatomic` (commonly macOS / some Clang setups). Consider
adding a CMake feature check and only linking `atomic` when required (e.g.,
when `std::atomic<__int128_t>` isn't lock-free / needs `-latomic`), rather than
always adding it to `tscore`'s public link interface.
--
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]
