fix(uring): halve the completion queue and lift the relay's memlock limit - #4197
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
MERGE Positive improvement, and the complexity is almost none. The 4096 CQ was sized for two sockets at the udp pool ceilings, but the relay and moq-bench already run one socket per worker. Halving to 2048 matches that layout, drops measured memlock from 88 KiB to 56 KiB per worker, and the test now asserts the invariant that actually matters. Overflow stays survivable via
Skipping a tunable is the right call here. A knob would not fix those hosts, and operators would not size better than "raise the limit." Doc follow-up for #4167 (~100 KiB -> ~56 KiB) is correctly left out of this PR. No wire or public API impact. Ship it. This is an automated review, not the maintainer's decision |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c0ac1180e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// No larger: the ring is charged to `RLIMIT_MEMLOCK` at 16 bytes per entry, | ||
| /// most of each worker's footprint, and that budget is shared by every | ||
| /// io_uring the user runs. | ||
| const CQ_ENTRIES: u32 = 2048; |
There was a problem hiding this comment.
When two default-configured sockets share a worker, each can contribute 1,024 send and 256 receive completions, so their 2,560 CQEs exceed this new 2,048-entry queue. This is a supported in-repo topology: rs/moq-uring/benches/session_lite.rs binds both its server and client sockets through the same handle at lines 115-121. Once concurrent sessions grow both pools, the worker enters the allocation-per-CQE overflow path and terminates armed multishot receives, undermining the benchmark and public multi-socket use; retain the two-socket capacity or enforce a one-socket limit and measure that change.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
…imit Each worker's ring is charged to RLIMIT_MEMLOCK, and the 4096-entry CQ was most of its 88 KiB. The CQ was sized for two sockets at their pool ceilings, but every consumer runs one socket per worker, so 2048 covers it at 56 KiB per worker (measured as a fresh uid). Shrinking alone cannot rescue hosts on the old 64 KiB default: even a 1024-entry CQ costs 40 KiB, one worker at most. The shipped systemd unit now sets LimitMEMLOCK=infinity instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#4167 documented each ring at about 100 KiB. This branch halves the completion queue, so that figure is 56 KiB. Co-authored-by: Grok 4.7 <noreply@x.ai>
8c0ac11 to
ad1c18a
Compare
|
Landing this on main. Rebased onto current The Codex note about two sockets on one worker is the old 4096 sizing. The relay and moq-bench run one socket per worker, overflow stays survivable via (Written by Grok 4.7) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad1c18a43d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| LimitNOFILE=1048576 | ||
| # io_uring workers charge their rings to RLIMIT_MEMLOCK (~56 KiB each), and hosts still on the old | ||
| # 64 KiB default fit one worker at most. | ||
| LimitMEMLOCK=infinity |
There was a problem hiding this comment.
The official package build at .github/workflows/moq-relay.yml:58-85 invokes cargo zigbuild ... -p moq-relay without --features io-uring, and that feature is explicitly absent from the defaults in rs/moq-relay/Cargo.toml:28-49, so the binary installed alongside this unit cannot create these workers. This therefore grants an unrelated network-facing process unlimited locked memory without fixing any packaged configuration; systemd-analyze syscall-filter @system-service also expands to @memlock, whose group includes mlock, mlock2, and mlockall, so the existing sandbox does not neutralize the raised limit. Either enable io_uring in the packaged binary or keep a bounded limit for this unit.
Useful? React with 👍 / 👎.
1 participant
Every io_uring worker's ring is charged to
RLIMIT_MEMLOCK, a per-user budget shared by every io_uring that user runs. On hosts still on the old 64 KiB default, no relay worker could start.Measured as a fresh uid (worker plus one socket):
CQ_ENTRIES4096 -> 2048. It was sized for two sockets at theirudp::Configpool ceilings (1280 in-flight ops each), but the relay and moq-bench run one socket per worker. Overflow stays survivable (IORING_FEAT_NODROP).packaging/moq-relay/moq-relay.servicesetsLimitMEMLOCK=infinity. Shrinking alone cannot fix 64 KiB hosts: even 1024 entries fits one worker at most.@system-servicealready allows the io_uring syscalls.Not made configurable: a knob would not fix those hosts, and nobody would tune it better than raising the limit.
Public API / wire impact: none.
The relay doc from #4167 said each ring costs ~100 KiB. This branch updates that line to ~56 KiB.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code