Repository navigation
quest(m1): plan lock-wait CPU from switches and the bench relay port race #5404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # [S] Bench harness knows its relay owns the port | ||
|
|
||
| ## Goal | ||
|
|
||
| `bench/relay.sh` reports a relay as ready only once that relay holds the port | ||
| it was given. A relay that loses the port fails the run at startup with the | ||
| relay's log, never as a load generator that silently receives zero groups. | ||
|
|
||
| ## Plan | ||
|
|
||
| `start_relay` picks a random port in 40000-59999, starts the relay, and treats | ||
| any TCP connect to that port as readiness. If another process already listens | ||
| there, the probe succeeds against it. The relay then exits with "Address | ||
| already in use", and the harness only notices when the load generator receives | ||
| nothing. This happened once on a busy host during #5277. | ||
|
|
||
| A shell harness cannot pick a port and bind it atomically for the relay, so | ||
| the open choice is where ownership gets proven. Settle it in the PR: | ||
|
|
||
| - Recommended: keep the random port, and after the probe succeeds confirm that | ||
| `$RELAY_PID` is alive and owns both the TCP and UDP listeners on it (for | ||
| example with `ss -lnp`). Otherwise retry with a new port. Bench-only, with no | ||
| relay change. | ||
| - Let the relay bind port 0 and report its bound addresses. The relay prints | ||
| neither address today, and QUIC and HTTP would get different ports, so this | ||
| adds relay behavior and docs for a bench-only problem. | ||
| - Probe a relay-specific endpoint, such as its certificate fingerprint. This | ||
| proves the HTTP listener but not the QUIC socket. | ||
|
|
||
| Cover it in `bench/relay.test.py` (already run by `just gh check`): hold the | ||
| chosen port open and expect the harness to retry or refuse. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Benchmark reporting](/bench/README.md) - the harness this fixes | ||
| - [Relay profiling](/quest/m1/perf/lock-profile.md) - where the race surfaced | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # [S] Lock-wait CPU from the recorded switches | ||
|
|
||
| ## Goal | ||
|
|
||
| `just bench-lock` measures each worker's CPU from the same perf recording | ||
| that measures its waits, so wait/CPU shares one clock and one window. | ||
| `/proc` schedstat only tells an idle worker from one that never switched. | ||
|
|
||
| ## Plan | ||
|
|
||
| Today `bench/profile.py record` reads schedstat before `perf record` starts and | ||
| after it exits, while perf records no switch-out samples for its first ~115 ms. | ||
| CPU therefore covers a longer window than the waits do, biasing wait/CPU about | ||
| 2% low on the 5 s window. A worker that ran only during perf's start-up and then | ||
| slept would be refused ("ran but never switched out"), although the recording | ||
| is valid (CodeRabbit on #5277). | ||
|
|
||
| Decided 2026-10-13 (maintainer AFK, recommended option taken): | ||
|
|
||
| - Derive CPU from the `--switch-events` records already in `perf.data`: sum | ||
| each worker's on-CPU intervals, switch-in to the next switch-out, clipped to | ||
| the window the samples cover. A worker's first record gives its state at the | ||
| window start: a switch-out means it was running. Schedstat no longer supplies | ||
| CPU. | ||
| - Rejected: gate perf through its `--control` FIFO and take schedstat at the | ||
| enable and disable points. It keeps two clocks and adds a control channel. | ||
| - A worker with no switch records in the window either slept or ran | ||
| throughout, and the recording alone can't tell which (Codex on #5404). Keep | ||
| the schedstat reads only to classify those workers: if its counter did not | ||
| advance, the worker is idle; otherwise refuse the capture as ambiguous. | ||
| A worker with switch records but no sampled switch-out still refuses. | ||
| - Bump `lock.json`'s `version`, since `cpu_ns` changes meaning. | ||
| - Extend `bench/profile.test.py` with a worker that runs only before the first | ||
| sample, and a check that CPU and waits share the window. Re-run | ||
| `just bench-lock fanout-heavy 16` and record the before/after wait/CPU in the | ||
| PR. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Relay profiling](/quest/m1/perf/lock-profile.md) - the recipe this corrects | ||
| - [kio channel contention](/quest/m1/perf/kio-channel-contention.md) - reads these wait/CPU numbers |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: moq-dev/moq
Length of output: 15392
Make the port-conflict test deterministic.
Keep a foreign TCP listener responsive while the relay child is alive. Verify that the harness does not accept that listener as ready.
Suggested plan wording
📝 Committable suggestion
🤖 Prompt for AI Agents