From 74df925288811f82d13a1ea4cc3edfb836d65cb7 Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Sun, 26 Jul 2026 05:58:51 -0700 Subject: [PATCH] Update: forbid sleeping on the dispatch path, and stop sleeping on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codestyle rule 5 banned `yield()` in AICPU spin-waits but said nothing about the host, so a sleep-based backoff on a mailbox poll read as acceptable. It is not: a sleep quantum lands on every dispatch that arrives mid-quantum, and no tuning recovers it, because a sleep interval on a general-purpose OS overshoots what was asked for — a 50 us request measures ~102 us on this box. Generalize the rule. A wait that a task's latency passes through may only spin or block on a wakeup primitive (semaphore, futex, condition variable, pipe/eventfd read). Initialization and teardown are exempt and may poll with a sleep, since a bring-up handshake or a shutdown drain costs a one-off wait rather than per-task latency. The boundary is "does a task wait behind this?" rather than which tier or file the wait lives in, so a host completion poll lands on the same side as an AICPU ticket lock while `close()` reaping lands on the other. `LocalMailboxEndpoint::run` was the one place in the hierarchical worker that broke this: it slept 50 us between `TASK_DONE` polls, on the critical path of every dispatch. Its sibling `CONTROL_DONE` wait in the same file already spins and samples child liveness against a `kChildLivenessPollPeriod` wall clock, so the dispatch wait now matches it. Liveness has to move off the iteration count for the same reason the old comment gave for using one — "~10 ms at the 50 us sleep below" — that mapping only held while the loop slept. Measured, 3 sub workers with no-op callables: before after run() median 150.2 us 50.7 us run() p90 152.3 us 51.6 us run() max 175.9 us 94.1 us burst, per task 106.0 us 11.0 us `kChildLivenessPollInterval` loses its last caller and goes with it. The trade is CPU: a parent thread now spins for as long as its child holds a task, so W concurrently-dispatched workers keep W threads busy (measured 2.98 cores for 3 outstanding dispatches). On the dispatch path that is what the rule asks for, and it does not degrade under core pressure — `tests/ut/py/test_worker` plus `test_hostsub_fork_shm.py` run 342 passed in 54.3 s pinned to 3 cores against 52.6 s unpinned. Removing the CPU cost as well needs a blocking wakeup primitive on both ends of the mailbox, which is tracked separately and is what the rule's "spin *or* block" is pointing at. Co-Authored-By: Claude Opus 5 --- .claude/rules/codestyle.md | 30 +++++++++++++++++++++- src/common/hierarchical/worker_manager.cpp | 26 +++++++++---------- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/.claude/rules/codestyle.md b/.claude/rules/codestyle.md index b06b2fb68c..5e5a38a887 100644 --- a/.claude/rules/codestyle.md +++ b/.claude/rules/codestyle.md @@ -29,7 +29,35 @@ 3. Prefer `volatile` decorator on struct members rather than volatile pointer casts unless necessary. 4. Avoid using pointer arithmetic with hardcoded offsets when `offsetof` is available. -5. **Never use `std::this_thread::yield()` or `sched_yield()` in AICPU spin-wait loops.** On the Ascend AICPU, yielding to the OS scheduler introduces unacceptable latency for tight spin-waits (ticket locks, CAS retries, etc.). Use an empty loop body or a bare architecture hint (`__asm__ volatile("yield")`) instead. +5. **No sleeping on the dispatch path — at any tier.** A wait that a task's + latency passes through may only ever *spin* or *block on a wakeup + primitive* (semaphore, futex, condition variable, pipe/eventfd read). + `sleep`, `yield`, and backoff timers are all forbidden there, on the host + as much as on the AICPU: a sleep quantum is added to every dispatch that + lands mid-quantum, and it is not recoverable by tuning — the sleep + interval on any general-purpose OS overshoots what you asked for. + + **Initialization and teardown paths are exempt** and may poll with a + sleep — a startup handshake or a shutdown drain costs a one-off wait, + not per-task latency. `_STARTUP_POLL_INTERVAL_S` in `worker.py` is the + sanctioned shape. + + The boundary is *"does a task wait behind this?"*, not which file it is + in. A mailbox poll waiting for `TASK_READY`, a completion poll waiting + for `TASK_DONE`, a ready-queue wait, an AICPU ticket lock or CAS retry: + all dispatch path. Registration, device bring-up, `init()` readiness + barriers, `close()` reaping: all exempt. + + On the AICPU specifically, **never** `std::this_thread::yield()` or + `sched_yield()` — yielding to the OS scheduler costs more than the wait + it replaces. Use an empty loop body or a bare architecture hint + (`__asm__ volatile("yield")`). + + When an idle spin is genuinely too expensive to leave running (a forked + child that would hold a core for its whole lifetime), the answer is a + blocking wakeup primitive, **not** a sleep — blocking costs no CPU and + wakes in single-digit microseconds, where a sleep-based compromise pays + latency on every dispatch to save CPU while idle. 6. **For cross-platform/platform-isolation preprocessor blocks, place the `__aarch64__` branch first.** Use this ordering pattern: ```cpp diff --git a/src/common/hierarchical/worker_manager.cpp b/src/common/hierarchical/worker_manager.cpp index f17c6b9567..979c42aa41 100644 --- a/src/common/hierarchical/worker_manager.cpp +++ b/src/common/hierarchical/worker_manager.cpp @@ -55,12 +55,8 @@ std::string format_digest(const uint8_t *digest) { return out; } -// Task-dispatch spin iterations between child liveness samples. Paired with -// the loop's 50 us sleep this is a ~10 ms sampling period. -constexpr int kChildLivenessPollInterval = 200; - -// Wall-clock period between child liveness samples on spin loops that do not -// sleep between iterations. +// Wall-clock period between child liveness samples. Every mailbox wait spins, +// so an iteration count would not map to a bounded wall time. constexpr std::chrono::milliseconds kChildLivenessPollPeriod{10}; std::string child_status_message(int child_pid, int status) { @@ -368,14 +364,17 @@ WorkerCompletion LocalMailboxEndpoint::run(Ring *ring, const WorkerDispatch &dis // Signal child process. write_mailbox_state(MailboxState::TASK_READY); - // Spin-poll until child signals TASK_DONE. A child that dies without - // publishing TASK_DONE would otherwise leave this loop spinning forever, - // so its liveness is sampled every kChildLivenessPollInterval iterations - // (~10 ms at the 50 us sleep below), amortizing the waitpid() syscall. - int poll_count = 0; + // Spin-poll until child signals TASK_DONE. The task's latency runs through + // this wait, so it never sleeps (codestyle rule 5). A child that dies + // without publishing TASK_DONE would otherwise leave the loop spinning + // forever, so its liveness is sampled on a kChildLivenessPollPeriod wall + // clock rather than an iteration count, which no longer maps to a wall + // time once the loop runs at spin speed. + auto next_liveness_check = std::chrono::steady_clock::now() + kChildLivenessPollPeriod; while (read_mailbox_state() != MailboxState::TASK_DONE) { - if (++poll_count >= kChildLivenessPollInterval) { - poll_count = 0; + auto now = std::chrono::steady_clock::now(); + if (now >= next_liveness_check) { + next_liveness_check = now + kChildLivenessPollPeriod; std::string death = check_child_death(); if (!death.empty()) { completion.outcome = EndpointOutcome::ENDPOINT_FAILURE; @@ -383,7 +382,6 @@ WorkerCompletion LocalMailboxEndpoint::run(Ring *ring, const WorkerDispatch &dis return completion; } } - std::this_thread::sleep_for(std::chrono::microseconds(50)); } // Inspect the child's error report before releasing the mailbox back