InkAtomicList empty-checks and head peeks bypass atomic loads

[phongn]

While working on #13555 (see also #13571) we audited all accesses to the lock-free list head (head_p). All mutations go through INK_QUEUE_LD / ink_atomic_cas, but a few read-only "peeks" use plain, non-atomic reads of the pointer field:

• INK_ATOMICLIST_EMPTY (include/tscore/ink_queue.h), used by ProtectedQueue::wait (src/iocore/eventsystem/ProtectedQueue.cc) to decide whether to cond_timedwait.
• AtomicSLL::head() and AtomicSLL::empty() (include/tscore/List.h).
• LogObject constructor/destructor reads of m_log_buffer (src/proxy/logging/LogObject.cc), though these run in single-threaded contexts.

Impact

These reads race with concurrent CAS writers, which is formally undefined behavior (and visible to TSAN). In practice the impact is bounded today: the pointer half is a single aligned 8-byte access on all supported targets, so it does not tear, and the one wait-gating use is a timed wait, so a stale answer costs at most one timeout period of event latency, not a lost wakeup.

Suggested fix

Convert the peeks to __atomic_load_n(&head.s.pointer, __ATOMIC_RELAXED) (or an inline helper next to INK_QUEUE_LD). This is free on every supported platform and removes the formal race without changing behavior.

Low priority; filing so the audit result does not get lost.

[phongn]

A follow-up audit of the atomic wrappers (ink_atomic.h) found more instances of the same family of problem — ordering that is correct on x86 TSO only. Folding them into this issue so they stay together.

1. INK_MEMORY_BARRIER / INK_WRITE_MEMORY_BARRIER are empty macros

Both are defined to nothing in include/tscore/ink_atomic.h (comment: "not used for Intel Processors which have sequential(esque) consistency"). Every use in the tree is therefore a no-op, including on aarch64. Five of the seven use sites are harmless because an adjacent seq-cst CAS/RMW already provides the ordering (ink_queue.cc x3 pushes, HttpConfig.cc, LogBuffer.h:222, LogObject.cc:374, InkVConnInternal.cc:132, Transform.cc:373). Two are load-bearing in intent:

• src/iocore/aio/AIO.cc:300 (aio_init_fildes): the barrier sits before the initialization writes it is meant to order. The publishing store num_filedes++ carries the comment "should be incremented after initializing everything. This prevents a thread from looking at uninitialized fields", but it is a plain store, the barrier is a no-op, and readers scan aio_reqs[] up to num_filedes with no acquire.
• src/iocore/net/UnixNetVConnection.cc:275 (do_io_close): VIO teardown and lerrno are published by a plain closed = 1 store guarded only by the empty macro. The in-code comment ("Must mark for closed last in case this is a cross thread migration scenario") documents the intended release ordering that the code does not actually have.

2. Acquire-only pointer publication via __sync_lock_test_and_set

ink_atomic_swap maps to __sync_lock_test_and_set, which GCC documents as an acquire barrier only. Several sites use it to publish freshly constructed objects, which requires release semantics:

• src/iocore/cache/CacheProcessor.cc:641 — vol_hash_table install after the table is filled with plain stores.
• src/proxy/CacheControl.cc:136 — new CC_table install.
• src/proxy/logging/Log.cc:131 — new LogConfig install (a comment there already documents a known race).
• src/proxy/logging/LogConfig.cc:334 — Log::error_log install.
• src/records/RecDebug.cc:37 — g_diags install.
• src/iocore/eventsystem/UnixEThread.cc:449 — tail_cb install, read cross-thread in UnixNetVConnection.cc:385.
• plugins/stats_over_http/stats_over_http.cc:1448 and plugins/compress/compress.cc:1009 — config pointer swap on reload.

On x86 the xchg instruction hides this. On weakly ordered targets the pointed-to contents are not formally ordered before the pointer becomes visible. Mapping ink_atomic_swap to __atomic_exchange_n(..., __ATOMIC_SEQ_CST) fixes all of these at zero cost on x86 (identical codegen).

3. LogBuffer reader side has no acquire

The writer side is fine: the log payload is written before the checkin_write CAS on m_state.ival (include/proxy/logging/LogBuffer.h:223), and the seq-cst CAS releases it. But the flush thread reads m_state with plain 64-bit loads (src/proxy/logging/LogBuffer.cc:266, :369) and then reads the payload with no acquire edge. Same class as the peek reads described above.

Suggested direction

Same as the original report: convert the peeks and the plain publishing/consuming loads to explicit __atomic operations with the appropriate ordering, and give the two load-bearing barrier sites real fences (or proper atomics). The empty barrier macros should then be deleted; their five decorative uses can go without replacement.