Copilot review on #15947 flagged that `volatile uint32_t` is not a
well-defined concurrent access in the C++ memory model — it prevents
the compiler caching/eliding the read, but does not turn a plain
cross-thread read/write pair into a defined access. Technically still
a formal data race even though aligned 32-bit LDR/STR on ARMv5TE is
atomic at the hardware level.
Switch the NO_ATOMICS counter reads and writes to GCC's atomic
builtins with __ATOMIC_RELAXED:
- Readers: __atomic_load_n(&counter, __ATOMIC_RELAXED)
- Writers (under lock_): __atomic_store_n(&counter, new_value,
__ATOMIC_RELAXED)
- Increment/decrement (under lock_): explicit load + compute +
__atomic_store_n. Lock_ serialises the load-modify-store against
other writers; the atomic ops make the write visible to concurrent
readers in the memory model.
On ARMv5TE these builtins compile to plain LDR/STR — same codegen as
the previous volatile approach, and no libatomic dependency (only RMW
builtins like __atomic_fetch_add would need the lib). ATOMICS and
SINGLE paths are unchanged.
Rename the seven counter RMW mutators to carry the `_locked_` suffix
that matches the existing convention (pop_raw_locked_,
is_item_removed_locked_, cancel_item_locked_, etc.):
to_add_count_increment_ -> to_add_count_increment_locked_
to_add_count_clear_ -> to_add_count_clear_locked_
defer_count_increment_ -> defer_count_increment_locked_
defer_count_clear_ -> defer_count_clear_locked_
to_remove_add_ -> to_remove_add_locked_
to_remove_decrement_ -> to_remove_decrement_locked_
to_remove_clear_ -> to_remove_clear_locked_
The caller-must-hold-lock contract became load-bearing when the
underlying counters became volatile on NO_ATOMICS: ++/+=/-- compile to
a three-instruction LDR/OP/STR sequence that is not atomic against a
concurrent RMW from another task, so the lock is what keeps the
counter consistent. The new suffix makes the requirement explicit at
every call site, matching how the rest of the scheduler documents the
same invariant.
No behavioural change; all call sites already hold lock_.
The _empty_() helpers (to_add_empty_, defer_empty_, to_remove_empty_)
forced the lock path on ESPHOME_THREAD_MULTI_NO_ATOMICS by hardcoding
`return false`. That made Scheduler::call() pay a FreeRTOS mutex
round-trip for each of process_defer_queue_ / process_to_add /
cleanup_ on every idle tick just to confirm "nothing to do".
On the only NO_ATOMICS target (BK72xx — ARMv5TE, single-core), an
aligned 32-bit load is atomic at the hardware level. Mark the three
skip-work counters volatile so the compiler cannot cache or elide the
read, and let _empty_() compare against zero directly. Writers still
hold lock_ for any RMW — that invariant is unchanged.
A stale 0 is benign: the counter is checked on every Scheduler::call()
iteration, so a missed update is caught next tick. Same pattern as the
NO_ATOMICS reads in time_64.cpp.
On BK72xx at ~3100 iter/min with ~8us/mutex this reclaims roughly
75ms/min of main-loop overhead. Measured on BK7238/BK7231N while
profiling alongside libretiny-eu/libretiny#360.
ATOMICS and SINGLE paths are unchanged (SINGLE keeps plain uint32_t,
no volatile-read overhead).
J. Nick Koston