Repository navigation
perf(sock): attribute UDP queue loss and keep the 8 MiB socket buffers - #7529
Merged
Merged
Conversation
Quest-Claim: 844d08f5c1c4c911
…y sampler Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The socket-queue sweep shows loss follows the kernel queue against worker stalls while pool depth, flows and the receive path do not change it, and relay shard sweeps see no socket drops. Record the evidence where the setting lives, share the pool finding with the io_uring pool quest, and finish the quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Raw evidence for the measurements in the description (nixos2, revision benchmark_socket_queues, 3 rounds, every rowjust bench-shard "0 1 4" "16 64 256"(Written by Claude Opus 5.5) |
kixelated
changed the title
kixelated
marked this pull request as ready for review
November 1, 2026 09:55
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
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. CodeRabbit Verify
|
kixelated
commented
Nov 1, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e1d3b60435bc443d190e02ebbfb896138caa53a6.
P2: Preserve CPU/RSS sampling when UDP inode enumeration is denied (rs/moq-bench/src/host.rs:249). A non-root sampler can read a service's stat/task/status files while its other-UID /proc/<pid>/fd directory is inaccessible. The new udp::inodes(&target.proc)? then fails the whole sample; retain_mut reports the still-running process as exited and terminates when it was the only target. Treat unavailable per-target UDP enumeration like the already-optional sock_diag dump: report its absence and retain ordinary samples. Cover an accessible target whose fd enumeration is denied.
Overall direction: keeping production buffer settings unchanged and adding private loss attribution is appropriately scoped; no public API or wire change is needed. The optional instrumentation should not narrow the existing sampler's usable targets.
Verification: static GitHub review, including an independent sampler check; no builds, tests, or benchmark measurements run. No earlier published review findings to carry forward.
(Written by OpenAI)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — two small robustness notes inline.
Reviewed changes — the measured no-win that keeps the relay's 8 MiB UDP buffer request and adds only benchmark/sampler instrumentation, docs, and quest cleanup.
rs/moq-bench/src/host/udp.rs(new): asock_diagnetlink dump matched against/proc/<pid>/fdinodes, emitting flattenedudp_*sampler fields;Dropskeeps a cumulative per-inode total across a 32-bit wrap and a closed socket.rs/moq-bench/src/host.rs: one dump per sampler round, passed to each target'ssample; a refused dump drops only the UDP fields, once and loudly.bench/summary.py/bench/run.sh: newudp()diffs the bracket samples so a drop is never split, and the shard table printssock-drops/gen-drops.rs/moq-uring/tests/integration/socket_queues.rs(new, ignored/nightly): accounts for every datagram across the kernel queue, pool depth, workers, flows, GSO and four load patterns on both receive paths.- Docs/quest: moq-uring, moq-bench and bench READMEs; the survey, shard-default and uring-pool-memory quests record the verdict and delete
socket-queues.md.
No production behavior, wire, API, or config change: the bind.rs hunk is a doc comment only.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
# Conflicts: # bench/summary.py
An unreadable /proc/<pid>/fd (another user's process) now drops only the UDP fields for that target instead of reporting it as exited. A short NLMSG_ERROR in the sock_diag dump is rejected instead of indexing past the datagram. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
On the OpenAI review P2, an unreadable fd table failing the whole sample: fixed in (Written by Claude Opus 5.5) |
kixelated
commented
Nov 1, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 45ee9c10bc004ccfde0c7bcbeb0311f69d4c05fc (incremental review since e1d3b60435bc443d190e02ebbfb896138caa53a6, accounting for the main-branch merge).
Fixed: the previous P2. udp::inodes preserves permission denial, and udp_sample now disables only the target's UDP accounting while CPU/RSS sampling continues; the new regression covers the subsequent sample. The independently reported short netlink error is also bounded by the message length and covered by a regression.
No new actionable bugs found in the update. The focused fallback and parser fix are appropriate, without expanding production API/configuration; the summary merge resolution preserves the existing UDP calculations.
Verification: static review of the changed code, surrounding sampling/parser paths, tests, and merge resolution on GitHub. No builds, tests, or benchmark measurements run.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: the incremental delta since the prior pullfrog review (e1d3b60435), covering the fd-denied fix, the bounded NLMSG_ERROR read, and the merge of origin/main.
- Kept sampling targets whose fd table is denied:
/proc/<pid>/fdreturningPermissionDeniednow drops only that target's UDP fields, once and loudly, instead of failing the whole sample and reporting the process as exited; any other fd error still means it exited. - Bounded the
NLMSG_ERRORcode read: the code is read throughdata[..len].get(16..20), so a malformed short error at the end of a datagram returnsInvalidDatarather than indexing past the slice. - Documented exact inode keying in
Dropsand mergedorigin/main(ruff reformat ofbench/summary.pyandbench/summary.test.py).
Both prior pullfrog threads are addressed (the panic is fixed; inode reuse is declined with the get_next_ino() argument), and the OpenAI P2 finding is resolved. python3 bench/summary.test.py and cargo test -p moq-bench (including denied_fd_table_drops_only_udp and short_error_is_rejected) pass.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
kixelated
enabled auto-merge (squash)
November 1, 2026 18:12
kixelated
disabled auto-merge
November 3, 2026 06:20
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
commented
Nov 3, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 9f5f356
No new actionable issue found. Accounting for main's removed shard sweep and completed pool quest, the socket-queue benchmark remains nightly and the shard sweep is documented as local-only. The earlier permission-denied and short-netlink fixes remain in place. The PR's opening summary still says both are nightly; its later Nightly section correctly distinguishes them.
Static GitHub review only; I did not run tests or reproduce measurements.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the incremental delta since the prior pullfrog review (45ee9c10): a merge of origin/main that resolves conflicts and folds in main's newer content. No change to this PR's own code, benchmark, or sampler.
- Merged
origin/mainwith conflict resolution:sh/gh/nightly.shkeeps both this PR'sbenchmark_socket_queuesline and main'sbenchmark_datagram_delivery(main'smoq-archiveline intact);bench/README.mdkeeps main's local-only wording alongside the newsock-drops/gen-dropsdescription; the surveyREADME.mdrecords the no-win verdict and drops the deletedsocket-queues.mdlink. - Deleted quests stay deleted:
quest/m2/perf/uring-pool-memory.md(finished on main by #7233) andquest/m2/performance-survey/socket-queues.md(finished by this PR); no remaining references to either.
Both prior pullfrog threads are resolved and the OpenAI P2 finding is fixed on the reviewed head.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Finishes quest/m2/performance-survey/socket-queues.md as a measured no-win: the relay keeps its fixed 8 MiB UDP buffer request and gets no new setting. This PR adds the benchmark and sampler instrumentation that produced the result and schedules both nightly.
Changes
benchmark_socket_queues(rs/moq-uring/tests/integration/socket_queues.rs, ignored, nightly inrust-test). It runs on both receive paths a relay uses: moq-sockrecvmmsgunder Tokio, and the moq-uring multishot ring. Receivers are a steeredmoq_sock::shard::bindgroup, so they start with the production buffer request, and every flow's short-header CID steers it to a known worker. The axes vary one at a time:rx_buffers_max(pool depth);Each row accounts for every datagram: sent, delivered,
sk_drops(viaSO_MEMINFO, the same counterSO_RXQ_OVFLreports), path drops (the shaper's overflow), and unattributed. Next to those it prints namespaceRcvbufErrors,softnet_statbacklog drops,rx_enobufs, peak queued bytes, p50/p99/max delay, goodput, and receive CPU per datagram. Rounds interleave the cells (SOCKET_QUEUES_RUNS, default 3).moq-bench-hostUDP accounting (rs/moq-bench/src/host/udp.rs). Asock_diagdump (whatss -uamreads; no privilege needed) is matched against the target's/proc/<pid>/fdsocket inodes. Per sample it addsudp_sockets, the grantedudp_rcvbuf_min/udp_sndbuf_min,udp_rx_queue_bytes, and the cumulativeudp_drops, which handles the wrapping u32 and keeps the counts of closed sockets. The namespaceudp_rcvbuf_errors/udp_sndbuf_errors/udp_in_errorscome along as a cross-check.bench/summary.pydiffs the samples that bracket the load window, so a drop is never split across the boundary. Every relay summary now carries these fields for the relay and the generator. The nightlybench-shardtable printssock-dropsandgen-drops. A target whose/proc/<pid>/fdthe sampler may not list (another user's process) keeps its CPU and memory samples and only loses the UDP fields, like a host that refuses the dump.Docs:
bind.rscites the measurement onUDP_BUFFER. The moq-uring, bench and moq-bench READMEs describe the benchmark and the fields. The epic README records the verdict.API: none. moq-bench is
publish = false, and the benchmark is a test. Wire: none. Configuration: none.Measurements
nixos2: 12 CPUs, kernel 6.18.56,
rmem_max16 MiB, load average about 3 (other tenants). Revision96b25dba7f(the later commits only change docs). Release build innix develop, 3 interleaved rounds. Command:Each cell offers 50k datagrams/s (480 Mbps) of 1200-byte datagrams for 1 s.
queue req=relaykeeps the production request.rcvbufis the granted kernel value: Linux doubles the request, so the relay's 8 MiB shows as 16384 KiB. Drops are listed per round. Delay is the median over rounds.How to read it:
rx_enobufsfired ~511 times and nothing was lost: starvation is not loss. Four workers split the backlog, which cut the default queue's drops to ~4%.rmem_alloc, used only while a backlog exists. The peak was 2.3 MiB in a 20 ms stall, and the idle cost is zero.unattributedwas 0 in every row. GSO trains show the GRO caveat: ~332 socket drops stood for ~5300 lost datagrams, 16 per coalesced skb. The shaped path put every drop on the path (~1955 shaper overflows, 0 socket drops). Once, 10 datagrams were dropped at the shaper's own default-sized socket, which shows as unattributed. NamespaceRcvbufErrorsalso moved with other tenants on the local host, so the per-socket counter is the one to trust.Relay level, same host:
just bench-shard "0 1 4" "16 64 256"(roomvideoand 1×Nfanout, shared runtime vs 1 and 4 Tokio shards):RcvbufErrorsover each windowThe shared-runtime fanout cases reported 0.05–0.12% application loss with zero socket drops, so that loss is not local queue overflow. Raw rows for both runs: evidence comment.
A local run on the loaded 24-CPU dev host (kernel 7.0,
rmem_max4 MiB, so the relay request was clamped to 4 MiB) matched the above. The one exception: the clamped queue dropped 14% in the 100 ms stall where nixos2's full grant dropped nothing. The existing warning already namesnet.core.rmem_maxfor that case.Nightly:
just gh nightly rust-testruns the benchmark (table in the job log; this group publishes no report).just bench-shardnow printssock-drops/gen-drops. It is a local diagnostic since moq-dev/moq#7603 removed the Nightly "Relay shard sweep" job.Choices made without asking
SO_MEMINFOpolling instead of parsingSO_RXQ_OVFL. Both read the same per-socketsk_drops. Polling needs no ancillary space on either production receive stack, so it does not touch the cmsg buffer that moq-dev/moq#7394 is resizing, and it adds no per-packet cost. The quest said to keep instrumentation private, so production code is unchanged. Rollover is handled per socket.sock_diag, not/proc/net/udp, because onlysock_diagalso reports the granted buffer the survey contract asks for ("record effective values").Follow-ups (for
/quest-mergeto plan)sk_drops, would keep its path attribution exact./metricscould export per-socketsk_dropsif an operator ever needs to attribute local loss in production. Nothing in this survey needed it, so it stays out of scope.Online review findings (iteration on
e1d3b60)/proc/<pid>/fdused to fail the whole sample, so another user's process was reported as exited.udp::inodesnow keepsPermissionDenieddistinct, andudp_sampledrops only that target's UDP fields, once and loudly; any other fd error still means the target exited. Covered bydenied_fd_table_drops_only_udp, and checked by hand against pid 1 (systemd) as a normal user: it now samples CPU and RSS without theudp_*fields.NLMSG_ERRORwhosenlmsg_lenis 16–19 at the end of a datagram indexed past the slice and panicked. The code read is now bounded by the message length. Covered byshort_error_is_rejected.get_next_ino()(sock_allocinnet/socket.c), a global counter that only repeats after 2^32 allocations. A reuse between two sampler rounds cannot happen, so keying byidiag_cookiewould add nothing. TheDropsdoc comment now says so.Checks
On head
9f5f356868, after merging origin/main945aec5a53. Conflicts:sh/gh/nightly.sh: kept main'sbenchmark_datagram_deliveryand this PR'sbenchmark_socket_queues.bench/README.md: kept main's statement that the shard sweep is local-only, not a nightly series, with this PR'ssock-drops/gen-dropsdescription.quest/m2/perf/uring-pool-memory.md: main deleted it (moq-dev/moq#7233 finished it). It stays deleted; the pool-depth finding is still inrs/moq-uring/README.md.quest/m2/performance-survey/socket-queues.md: main edited it. It stays deleted, because this PR finishes it.Two decisions landed on main since the last head. Neither contradicts this PR:
UDP_BUFFERon main is still 8 MiB.rust-testignored benchmarks that this PR's benchmark joins.Results:
quest gate nix develop --command just check origin/mainpassed: rs, bench, markdown, gh, quest, shell and the rest. Rust tests: 11425/11425. Flake was skipped (this host's Nix builder cannot create user namespaces).quest check --base origin/mainpassed (552 documents).SOCKET_QUEUES_RUNS=1 … benchmark_socket_queuespassed on the merged head. This host'srmem_maxis 4 MiB, so the relay request was clamped there.Review
quest review), gpt-6.1-sol, medium effort.9f5f3568682096cc58bab6a6b690a8d00ff27df3, round 1 of a new session, against origin/main945aec5a5391.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code