Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3342181142
##########
src/tscore/ink_queue.cc:
##########
@@ -222,10 +251,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();
Review Comment:
The PR changes freelist/atomiclist pop semantics by introducing mutex-based
mutual exclusion (`freelist_new()` / `ink_atomiclist_pop()`), primarily to
address a subtle data race. The added unit tests exercise only single-thread
behavior and won’t catch regressions in the multi-threaded paths (e.g., the
original stale-head retry scenario described in #11640). A small stress test
that runs concurrent push/pop under TSan (or at least checks invariants like no
lost/duplicated nodes) would better validate the fix.
##########
cmake/Check128BitAtomic.cmake:
##########
@@ -0,0 +1,58 @@
+#######################
+#
+# 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.
+#
+#######################
+
+# Check128BitAtomic.cmake
+#
+# This will define the following variables
+#
+# TS_HAS_128BIT_ATOMIC
+# TS_NEEDS_MCX16_FOR_128BIT_ATOMIC
+#
+
+set(CHECK_PROGRAM
+ "
+ #include <atomic>
+
+ int main()
+ {
+ std::atomic<__int128> x{0};
+ __int128 expected{x.load()};
+ return x.compare_exchange_strong(expected, 10);
+ }
+ "
+)
+
+include(CheckCSourceCompiles)
+check_cxx_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_ATOMIC)
+
+if(NOT TS_HAS_128BIT_ATOMIC)
+ unset(TS_HAS_128BIT_ATOMIC CACHE)
+ set(CMAKE_REQUIRED_FLAGS "-Werror -mcx16")
+ check_c_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_ATOMIC)
+ set(NEED_MCX16 ${TS_HAS_128BIT_ATOMIC})
+ unset(CMAKE_REQUIRED_FLAGS)
+endif()
+
+set(TS_NEEDS_MCX16_FOR_128BIT_ATOMIC
+ ${NEED_MCX16}
+ CACHE BOOL "Whether -mcx16 is needed to compile 128bit atomics"
+)
+
+unset(CHECK_PROGRAM)
+unset(NEEDS_MCX16)
Review Comment:
`Check128BitAtomic.cmake` mixes C and C++ compile checks. The fallback path
uses `check_c_source_compiles()` even though the probe is C++ (`<atomic>`,
`std::atomic`), so the check will fail with the C compiler and incorrectly
disable `TS_HAS_128BIT_ATOMIC` (and mis-detect `-mcx16` need). Use
`CheckCXXSourceCompiles` and `check_cxx_source_compiles()` consistently, and
initialize/clean up the `NEED_MCX16` variable explicitly.
##########
src/tscore/ink_queue.cc:
##########
@@ -137,13 +144,26 @@ 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 != 0 && (alignment & (alignment - 1u)) == 0);
+
+ // Freelist nodes have to be at least void* aligned since they store void*s
when
+ // not allocated out.
+ alignment = std::lcm(alignment, alignof(void *));
+
Review Comment:
`ink_freelist_init()` allocates the `InkFreeList` object itself using the
caller-provided item alignment (after lcm with `alignof(void*)`). After this
PR, `InkFreeList` contains `std::mutex` and `std::atomic<head_p>`; on some
platforms (notably when `head_p` is `__int128_t`), `alignof(InkFreeList)` can
exceed `alignof(void*)`. If `alignment` is smaller than `alignof(InkFreeList)`,
placement-new constructing `InkFreeList` into under-aligned memory is UB.
--
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]
