Convert ink_queue implementation to std::atomic

ci/asan_leak_suppression/unit_tests.txt

@@ -2,3 +2,8 @@
 # not destroyed because it holds a reference to a stack-allocated
 # TestRefCountObj whose free() override calls exit(1).
 leak:test_http_hdr_print_and_copy_aux
+
+# on 64-bit architectures, freelist pointers are stored in a special bitwise
+# format, so LSan cannot find link pointers. It will erroneously determine
+# that the list tail is not reachable.
+leak:freelist_new

cmake/Check128BitCas.cmake

@@ -25,16 +25,18 @@
 
 set(CHECK_PROGRAM
     "
-    int main(void)
+    #include <atomic>
+
+    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)

include/tscore/ink_queue.h

@@ -37,6 +37,10 @@
 #include "tscore/ink_defs.h"
 #include "tscore/ink_apidefs.h"
 
+#include <atomic>
+#include <cstring>
+#include <mutex>
+
 /*
   For information on the structure of the x86_64 memory map:
 
@@ -71,30 +75,49 @@ 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;
+// 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;
 };
 
+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;
+}
+
+inline void
+store_head(head_p &dest, head_p_view const src)
+{
+  static_assert(sizeof(dest) == sizeof(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
@@ -119,17 +142,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
  *
@@ -144,16 +163,14 @@ union head_p {
  */
 #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
  *
@@ -163,28 +180,34 @@ union head_p {
  */
 #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
 
 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;
+
+  // These fields must be initialized once and not modified after
+  // initialization.
+  uint32_t type_size, chunk_size, alignment;
+
+  std::atomic<std::uint32_t> allocated_base;
+  std::atomic<std::uint32_t> used_base;
+  std::atomic<std::uint32_t> hugepages_failure;
+  bool                       use_hugepages;
+  int                        advice;
 };
 
 using InkFreeListOps = struct ink_freelist_ops;
@@ -213,9 +236,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_p> head{};
+  const char         *name   = nullptr;
+  uint32_t            offset = 0;
 };
 
 #if !defined(INK_QUEUE_NT)

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 *

src/tscore/CMakeLists.txt

@@ -161,6 +161,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