Skip to content

Make boottime and hz access atomic - #393

Merged
biscout42 merged 1 commit into
mainfrom
boottime-atomics
Aug 18, 2026
Merged

Make boottime and hz access atomic#393
biscout42 merged 1 commit into
mainfrom
boottime-atomics

Conversation

@nicholasberlin

@nicholasberlin nicholasberlin commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

quark.boottime and quark.hz live in a process-wide global shared by every queue. quark_update_boottime() may be called from one thread while another converts timestamps through quark_time_to_wallclock() — as auditbeat does with two independent quark consumers in the same process — which no caller-side lock can span. By the letter of C11 the plain u64 loads and stores are a data race.

Route all access through static inline helpers that use __atomic_load_n/__atomic_store_n with relaxed ordering. Relaxed is sufficient since boottime is a single self-contained value with no ordering dependency on other data; readers just need an untorn value and tolerate a stale epoch for one cycle.

To be clear about what this does and doesn't change: the compiled code is identical before and after — on x86-64 and aarch64 an aligned 8-byte access is already a single atomic mov/ldr/str, the helpers inline away, and there is no libatomic dependency (nm clean). Nothing misbehaves today and this fixes no observed bug. What the change buys:

  • It turns "correct by ISA coincidence and current compiler behavior" into "correct by contract". Plain access to a shared object licenses the compiler to cache, hoist or split it; today nothing bites only because every access sits behind an opaque call boundary. LTO, a future compiler, or an innocent refactor could change that silently, and the failure mode would be a single garbage timestamp during an NTP step — unreproducible.
  • It makes the documented contract satisfiable. quark(7) says state must be synchronized by the user, but the epoch spans consumers that don't know about each other, so no user lock can exist. With this change the three calls can be documented as thread-safe from any thread, which Document the boottime calls #388 now does.
  • The C side becomes TSan-clean, and the helpers plus struct comment make the sharing model explicit for the next reader instead of leaving it to be rediscovered.
  • On any 32-bit target a u64 store really is two instructions, so the tear would be real there. Not our platforms today; permanently a non-issue after this.

Also verified to build on centos7 gcc 4.8.5 — the __atomic builtins date to gcc 4.7, whereas C11 _Atomic/stdatomic.h would need 4.9, hence the builtins.

Must merge before #388, which documents the thread-safety this change provides.

@biscout42 biscout42 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM;

Thanks for the PR description, it is helpful and easy to read.

quark.boottime and quark.hz live in a process-wide global shared by
every queue. quark_update_boottime() may be called from one thread
while another converts timestamps through quark_time_to_wallclock(),
as auditbeat does with two independent quark consumers in the same
process, which no caller-side lock can span. The plain u64 loads and
stores were a data race.

Route all access through static inline helpers that use
__atomic_load_n/__atomic_store_n with relaxed ordering. Relaxed is
sufficient since boottime is a single self-contained value with no
ordering dependency on other data; readers just need an untorn value
and tolerate a stale epoch for one cycle. On x86-64 and aarch64 the
generated code is unchanged (plain mov/ldr/str, no libatomic), and the
builtins compile back to gcc 4.8.5 on centos7.

quark_update_boottime(), quark_get_boottime() and
quark_time_to_wallclock() are now safe to call from any thread.
@biscout42
biscout42 merged commit e83b2c2 into main Aug 18, 2026
2 checks passed
@biscout42
biscout42 deleted the boottime-atomics branch August 18, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants