phongn commented on issue #13572:
URL:
https://github.com/apache/trafficserver/issues/13572#issuecomment-5347296621
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.
--
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]
