Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3276626643
##########
src/tscore/ink_queue.cc:
##########
@@ -517,117 +578,136 @@ ink_freelists_dump(FILE *f)
void
ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t
offset_to_next)
{
+ // The pointers we push onto the atomiclist will also need to be aligned. If
+ // the offset is not aligned, then it is not possible for the caller to
+ // determine a consistent, safe alignment for the object the head_p objects
+ // are subobjects of.
+ ink_release_assert(offset_to_next % alignof(head_p) == 0);
+
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);
+
+ void *ret = TO_PTR(FREELIST_POINTER(item));
+ head_p *ret_ = reinterpret_cast<head_p *>(reinterpret_cast<unsigned char
*>(ret) + 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(ret, 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(item, 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);
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);
Review Comment:
`ink_atomiclist_push` constructs and writes a `head_p` into the item at
`offset` (`new (... + offset) head_p{}`), which assumes the link storage is
`sizeof(head_p)` and aligned for `head_p`. Many existing list node types
provide only a pointer-sized `next` field (e.g. `SLink<C>::next`), so this can
corrupt memory when `head_p` is wider than a pointer (128-bit CAS) and is still
UB due to type/lifetime mismatch even when sizes match. Consider storing only a
`void*` next pointer in the node and reserving `head_p` for the atomic
head/version.
##########
src/tscore/unit_tests/test_ink_queue.cc:
##########
@@ -0,0 +1,221 @@
+/** @file
+
+ Freelist and Atomiclist tests
+
+ @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 <cstddef>
+#include <cstdint>
+#include <thread>
+#include <vector>
+
+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]")
+{
+ struct test_type {
+ std::int32_t i;
+ head_p next;
Review Comment:
This test defines the atomiclist link field as `head_p next;`, but most
production uses of `InkAtomicList` (e.g. via `AtomicSLL` / `SLink<C>::next`)
use a pointer-typed `next` field. With the current implementation treating the
node link storage as `head_p`, these tests won't catch the
incompatibility/memory corruption risk on 128-bit `head_p` platforms. Please
add coverage using a node type whose `next` member is a pointer (or a real
`SLink`/`Link` member) to match existing call sites.
##########
src/tscore/ink_queue.cc:
##########
@@ -517,117 +578,136 @@ ink_freelists_dump(FILE *f)
void
ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t
offset_to_next)
{
+ // The pointers we push onto the atomiclist will also need to be aligned. If
+ // the offset is not aligned, then it is not possible for the caller to
+ // determine a consistent, safe alignment for the object the head_p objects
+ // are subobjects of.
+ ink_release_assert(offset_to_next % alignof(head_p) == 0);
+
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);
+
+ void *ret = TO_PTR(FREELIST_POINTER(item));
+ head_p *ret_ = reinterpret_cast<head_p *>(reinterpret_cast<unsigned char
*>(ret) + l->offset);
+ SET_FREELIST_POINTER_VERSION(*ret_, nullptr, 0);
+ return ret;
Review Comment:
These atomiclist operations treat the per-node "next" field as storage for a
`head_p` object (e.g. `new (... + offset) head_p{}` and
`reinterpret_cast<head_p*>`), but existing users pass offsets to pointer fields
such as `SLink<C>::next` / `Link<C>::next` (see `include/tscore/List.h`) and
`Event::link.next` (used by `ProtectedQueue`). This violates C++ object
model/strict-aliasing even when `head_p` is 64-bit, and it will overwrite
adjacent memory when `TS_HAS_128BIT_CAS` makes `head_p` 128-bit (16 bytes)
while the link field remains an 8-byte pointer. Please either (a) keep node
link fields as pointer types and only store a pointer there (versioning stays
in the list head), or (b) change the API contract to require `head_p`-typed
link storage and update all call sites/struct layouts accordingly.
##########
src/tscore/unit_tests/test_ink_queue.cc:
##########
@@ -0,0 +1,221 @@
+/** @file
+
+ Freelist and Atomiclist tests
+
+ @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 <cstddef>
+#include <cstdint>
+#include <thread>
+#include <vector>
+
+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)};
Review Comment:
Inside the first `SECTION`, `InkFreeList *f{...}` is redeclared and shadows
the `f` created at the start of the `TEST_CASE`. Because Catch2 re-runs the
outer scope for each section, this results in an extra freelist allocation in
that section that is never used (and never reclaimable), which can skew the
benchmark/leak behavior. Please reuse the existing `f` from the test scope
instead of creating a second one here.
--
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]
