Skip to content

counters: a read copies its records out in one write - #1160

Merged
Japabu merged 3 commits into
mainfrom
liveness
Nov 4, 2026
Merged

Japabu merged 3 commits into
mainfrom
liveness

Conversation

@Japabu

@Japabu Japabu commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On loaded hosts test_rs_counters_read (tests/images/virtsmpcase.toml) goes quiet past the harness's
15 s GUEST_QUIET. virt_smp, virt_el1_smp, virt_mask_windows and
virt_off_names_the_cpus_left_on then red as STALLED
(issues/a-counters-read-under-host-load-can-go-silent-for-15-s.md).

What a silent guest is doing

I captured silent guests over QEMU's gdbstub while the harness watched the silence, using a scratch
harness hook that is not committed. The kernel was main's, on TCG (virt_smp) and HVF
(virt_el1_smp):

  • Every vCPU is in Lock<AddressSpace>::lock, called from counters::read → View::pieces
    while writing the records out.
  • The lock moves. Its now, read a few seconds apart during the silence, advanced about 180
    handovers a second. ticket − now stayed 8, so every vCPU was queued.
  • The guest finishes the read after the harness gives up. In one capture the lock read zero at the
    last probe because the process had ended and its space was freed.

This is a convoy, not a deadlock. A ticket lock hands over to one particular waiter. When the host
has that waiter's vCPU descheduled, every vCPU waits for the host to schedule it. A round's sixteen
readers wake together, and each read took the process's space lock nine times: one window check
and one write_at per CPU record.

#1150's scratch capture was read as "a holder on no CPU", but it does not establish that. Tickets
are taken in time order, and its context table was printed after now had passed the tickets it
named.

Change

counters::read encodes every record and copies them out once. A read now takes the space lock
twice instead of nine times. The bytes written are the same.

Evidence

The results below and the complete capture logs are in the evidence branch: pr-1160/r0.

Unclamped, beside other guests. Six interleaved rounds of the four rows ran at host load 18 to 51 from other
worktrees on the 14-core Apple-silicon dev host. Spans are guest clock from counters_read's
TEST_START to its TEST_END.

kernel rows silent spans of the reads that ended
main e3a421563 (the control: this change reverted) 8 of 24 0.85 to 14.54 s
this change 2 of 24 0.87 to 12.55 s
main with a scratch test-and-test-and-set sync::Lock 0 of 24 0.76 to 1.57 s

The third arm changes only how the lock hands over, which makes the FIFO handover the cause. This
change makes the queue shorter, but the queue remains.

A starvation instrument. Running QEMU under taskpolicy -c background stages the silence
deterministically while ambient load stays low. With the harness and QEMU clamped, main went silent in
15 of 15 valid rows. This change went silent 2 of 8, and its finished reads took 6.5 to 13.7 s. One main
boot is excluded: the clamped firmware passed its 60 s UEFI watchdog and reset. At ambient load above 45
the clamp is too harsh, and boots time out before the kernel starts. Those rounds were discarded.

What remains, and where it is fixed

The convoy itself stays: ticket locks are kept, as decided on 2026-11-04 (FIFO fairness is what
metal gets, and QEMU must never reduce it). The silence is fixed where it really lives, in the
harness, which runs an 8-vCPU guest beside others on a host that cannot give it eight cores. That
is a follow-up PR: guests admitted by host CPUs, and GUEST_QUIET's stale doc comment fixed, with
the bound itself still unscaled. The issue record states that exit.

Checks

  • cargo run -- --ci host on 51160f934's tree: exit 0, 109 steps green (host-ci.log.gz). The later commit
    edits only the issue record's prose, so it changes nothing those steps built.
  • Guest: the A/B rows above, cargo test --test toyos-build -- virt_smp virt_el1_smp virt_mask_windows virt_off_names_the_cpus_left_on --nocapture, each round's log with its exit code in ab-unclamped/. The
    "this change" arm is fix-one-copy.patch, which differs from the commit only in a comment's wording and
    a blank line between use groups.
  • Negative control: the "main" arm, which is this change reverted, run interleaved at the same load.

Metal: none. The change alters only how the same bytes are copied, and counters_read asserts
every record on the guest.

🤖 Generated with Claude Code

Japabu and others added 2 commits November 4, 2026 00:00
A read wrote each CPU's record with its own `write_at`, and each copy
takes the caller's space lock to reach and pin the page. A round's
readers all wake together when it completes, so with sixteen readers on
eight CPUs every round queued 128 takes of one process's space lock back
to back, on top of each read's window check.

That queue is what silences `test_rs_counters_read` on loaded hosts.
Captured over the gdbstub of silent 8-CPU `virt` guests on main's
kernel, under TCG and HVF: every vCPU is in `Lock<AddressSpace>::lock`
from `counters::read` -> `View::pieces`. The lock's `now` advances about
180 handovers a second through the silence, and `ticket - now` stays 8.
Nothing is deadlocked. A ticket lock hands over to one particular
waiter, and when the host starves that waiter's vCPU every CPU waits for
the host to schedule it.

Encoding the records first and copying once takes the space lock twice
per read instead of nine times. Beside other guests at host load 18 to
51, six interleaved rounds of the four `virtsmpcase` rows: main went
silent 8 of 24, this 2 of 24. A scratch test-and-test-and-set lock went
silent 0 of 24, which leaves the remaining convoy to a lock-policy
decision that the issue records.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ess's

Ticket locks are kept, as decided on 2026-11-04: their FIFO fairness is what
metal gets. The record now names the exit where the silence lives: the
harness admits a guest only where the host has a CPU for each of its vCPUs,
and the four virtsmpcase rows then pass under the load that silenced them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Review of #1160 at 51160f934 (reviewer, read-only)

Architecture. The change sits in the right place and is small. counters::read owns the copy, and encoding every record into one buffer before a single write_at is the direct way to stop each reader from re-taking its space lock once per CPU. It brings in no new mechanism. The record is correct to leave the real residual, FIFO ticket handover under a host that starves the guest, as a lock-policy choice for the owner and not to settle it here. Net: kernel +7/−3, issue record −222/+44. No test is added, and none is needed: the same bytes are visible by inspection, and counters_read already asserts every record on the guest.

What I checked.

  • Same bytes. Before, write_at(cpu * RECORD_BYTES, encode(cpu)) ran for cpu in 0..cpus. Now the 0..cpus encodings are concatenated in order and written at 0. That is the same range with the same contents. The only difference is that every snapshot is taken before the copy, where before the snapshots and copies alternated. Each record still reads served before its snapshot, so the freshness contract is unchanged.
  • Allocation. It is cpus * RECORD_BYTES (176 B per CPU). cpus is smp::cpu_count(), which smp.rs caps at MAX_CPUS, and out.len() >= cpus * RECORD_BYTES is checked first. The size is kernel-determined, bounded and not user-controlled.
  • Copy outside spin locks. By the time of the write, LAST's guard and answer's IrqGuard are both dropped, and sys_counters holds nothing around the call. View::pieces takes the space lock only to reach and pin pages, then copies under preempt_off, consistent with S0-a's gate. 2 × 176 × 8 = 1408 B fits in one CHUNK_PAGES (16) batch, so one pieces take plus the window check gives the PR's "twice".
  • Evidence. Host CI ran on the committed tree. The A/B ran with the reverted change interleaved as the control, and the "this change" arm's patch differs only in comment text. That is enough for a copy-path change. I agree a metal readback is not needed for correctness.

Required

  1. issues/a-counters-read-under-host-load-can-go-silent-for-15-s.md:13-15 — the record says "sighted eleven times from 2026-10-04 to 2026-11-02". The eleven sightings in the record this replaces are at bc5f36c7b, 0c0a348e1, 68cf1f870 (×2), 94fd25e71, b9430ed61, 22b7bb464 (×3) and aa7448db0 (×2), all committed between 2026-10-04 and 2026-10-10. 2026-11-02 is the date of kernel: user copies go through the process's own mapping, and none runs under a lock (S0-a's lock half) #1150's scratch reading, which is not a sighting. Either the end date is wrong or later sightings are being counted without being named. Correct the span, or cite the later sightings and fix the count.

Optional

  • kernel/src/counters.rs:256-257 — "which every reader of a round wants" holds only for readers in the caller's process. Readers in other processes take their own space locks. Suggested wording: // One copy: each copy takes the caller's space lock, which its other readers of the round want at the same moment.
  • The record's residual is waiting on an owner decision, but kind: defect/status: open keeps it out of rg '^status: owner'. A separate kind: question for the lock-policy choice (TTAS everywhere vs. per-machine selection vs. accepting the convoy) would put it in the owner's queue, with this defect pointing at it. The orchestrator can also simply raise it directly.

Verdict: SEND BACK (one record-accuracy fix; the code is ready).

… comment names whose lock

The eleven sightings all fall between 2026-10-04 and 2026-10-10; the
2026-11-02 reading of #1150's scratch kernel was not one of them. The copy's
comment names the readers that want the lock: the caller's own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu
Japabu marked this pull request as ready for review November 4, 2026 05:34
@Japabu
Japabu enabled auto-merge November 4, 2026 05:34
@Japabu
Japabu added this pull request to the merge queue Nov 4, 2026
Merged via the queue into main with commit 65c77e6 Nov 4, 2026
10 checks passed
@Japabu
Japabu deleted the liveness branch November 4, 2026 05:53
Japabu added a commit that referenced this pull request Nov 4, 2026
…aves out is recorded

`a-counters-read-under-host-load-can-go-silent-for-15-s` asked for the
harness to admit a guest only where the host has a CPU for each of its
vCPUs, and for the four virtsmpcase rows to pass under the parallel load
that silenced them. Two harness invocations of the four rows ran at once,
beside other worktrees' work at host load 19 to 53, over four interleaved
rounds. Main (with #1160) went silent in 5 of 32 rows, and its reads took up
to 15.6 s. With the budget, no row of 32 went silent and 28 reads of 32
took under 2.4 s.

One budgeted read still took 14.22 s with only its own guest admitted:
the budget counts guests, and builds and other non-guest work are not in
it. `the-guest-cpu-budget-counts-guests-and-not-builds` records that.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant