Repository navigation
feat(rccl): attribute slow collectives to fabric or host - #13876
Draft
dhanavikram wants to merge 19 commits into
Draft
dhanavikram wants to merge 19 commits into
dhanavikram wants to merge 19 commits into
Conversation
github-actions
Bot
added
documentation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
nikitaxgusev
reviewed
Nov 3, 2026
nikitaxgusev
left a comment
nikitaxgusev
left a comment
Contributor
There was a problem hiding this comment.
Tested on Ruby (2 × 8 MI350X, Broadcom Thor2 / brcmrdma24, FW 232.0.219.0): pool fix works (0 drops vs 59% on develop), incast correctly flagged LINK-SHARED, per-collective bytes within 2% of the whole-run total, and all unit + Python tests pass. Found one correctness bug in the ECN verdict and a few Thor2-specific issues with counter names and port counter cadence — details inline.
…OMNET-432) - thor2 table: read the counter names current bnxt_re drivers expose (tx/rx_cnp_pkts, rx/tx_roce_errors, rx/tx_roce_discards, res_mem_err, rx_out_of_buffer), keeping the old names as fallbacks. These fields read -1. - PFC fields read rx/tx_pfc_frames instead of global pause frames. - ethtool reader: a counter's primary key now wins over its fallback; before, whichever the NIC listed first won. - IB-CAST: count CTS completions in num_wqe_rcvd, all sub-requests of a multi-send in tx_bytes, zero-length WQEs in wqe_size_stats, and pre-posted receive WQEs; give QP-sharing secondary senders telemetry slots. - JSON transport is "none" when no device registered. - Unit tests: ethtool fallback order, thor2 table against a fake sysfs tree.
- rx_roce_error_pkts / rx_roce_discard_pkts no longer fall back to the same file as rx_errors / rx_discards, so thor2 errors are not reported twice. - PFC fields read only rx/tx_pfc_frames; global pause is not a fallback. - Drop the thor2 local_ack_timeout_err row (no such counter on bnxt_re) so the hw_samples series uses to_retransmits. - Count CTS completions only for active requests, like data receives. - Set isDataQp on QP-sharing secondary copies instead of special-casing it. - Test: remove its temp sysfs tree, honor TMPDIR. - Docs and stale comments updated.
A QP-sharing secondary maps qps[q] to shared QP q % primaryNqps, so with more entries than the primary two entries point at one QP. Completions are matched by qp_num to the first, so give repeats the first entry's slot instead of their own. Applies to senders and receivers. Also: thor2 errors/discards rows read the current bnxt_re name first; one comment and one test comment updated; resolver condition on one line.
- rcclTelemetrySetupQps gives each distinct QP of a device one slot; the sender and receiver setup in connect.cc share one helper that calls it. Replaces IbCastTelSameQp and is unit tested. - Keep the thor2 local_ack_timeout_err row (-1); the hw_samples sampler uses the alternate counter when a row cannot be read. - Test-only declarations move to net_telemetry_internal.h. - Tests: primary sysfs key wins when both names exist, and a device with only old bnxt_re names reads them as fallbacks. - Docs: error completions are not in num_wqe_rcvd; hw_samples fallback.
…e (AICOMNET-432) - A sampled column with both a primary and an alternate counter retries the primary for up to RCCL_TEL_SAMPLE_PRIMARY_TRIES samples, locking on its first good read, else to the alternate. One failed first read no longer switches the whole run to the alternate. - Test-only declarations move back into net_telemetry.h, in a marked internal section; net_telemetry_internal.h is removed. - Unit test for the retry and lock behaviour.
…T-432) bnxt_re's sysfs rx_bytes is 48 bits wide. When it wrapped during a run, delta_rx_bytes came out as a large negative number. A counter that went down is now taken as one 48-bit wrap, or reported as unknown (-1) when it cannot be one. Applies to every HW delta (hw_counters, PFC, delta_*).
…-432) Only bnxt_re's sysfs byte/packet counters are known to be 48 bits wide, so only those deltas get the 48-bit wrap. Any other counter that goes down (a 32-bit wrap or a reset) now reads -1 instead of a ~2^48 delta. Also skip NULL QPs in rcclTelemetrySetupQps, warn once on stderr when QPs get no telemetry slot, and document the multi-send byte count.
…AICOMNET-432) The sampler resolves the primary and alternate rows separately now, the first wanted ethtool key wins over the ethtool stat order, and the JSON transport is "none" when no device names one.
RCCL can enqueue hundreds of collectives ahead of the GPU, and the proxy thread starts every queued collective's proxy ops right away. On a 2-node ring that is 12 ops per collective, so about 85 queued collectives filled the fixed 1024-slot proxy-op pool. Every later collective was written with n_proxy_ops 0 and no network data: 1624 of 3009 records on one rank of a 2-node all_reduce_perf run. Each pool now starts with one chunk of the old size and adds chunks when it runs out, up to a cap (coll 4096, proxy op and step 65536). Chunks never move, so handles stay valid. Free slots sit on a list, so alloc and free are O(1). The summary's *_pool_size fields report the caps.
Correlates per-collective accl timing with RCCL net telemetry NIC counters to tell whether a slow collective is the fabric's fault (rx PFC pause: the switch paused us) or the host's (tx pause: we paused the switch). net telemetry: - hw_samples sampler also records per-priority PFC pause time/frames, per-priority bytes and port tx/rx bytes/packets (cached ethtool lookups, only for devices with a snapshot), plus link speed and sampler metadata. - Thor2 samples come from ethtool port counters (sysfs refreshes irregularly); the delta counters are unchanged. accl profiler: - Captures rcclTelemetrySwCapture snapshots over each collective's network window (first data-moving proxy step to last proxy op stop) and writes per-NIC deltas, host CLOCK_MONOTONIC timestamps, host, pid and comm hash. ACCL_PROFILER_NET=0 turns it off. accl_fabric_report.py correlates both outputs into a text report (FABRIC / HOST / LINK-SHARED / INCONCLUSIVE per busy stretch) and a Perfetto trace. tools/net-counter-probe reads the same NIC counters standalone.
How to build and run the accl profiler with network telemetry, make the report and trace, read the findings, and check that the counters show the right values.
…iorities Say when RCCL's priority has no PFC counters (PFC is off on it, so congestion shows as loss and CNPs), and print counter refresh times under a second in ms instead of rounding them to 0 s.
A NIC sends at most one CNP per flow every few tens of microseconds, so CNPs as a share of packets stayed near 0.2% on a congested mlx5 fabric and the 1% threshold could never fire. Report ECN as a share of received packets and CNPs (sent plus received) per second, each as a lower-upper range with the usual yes/maybe levels, and count CNPs the NIC sent, which was the only signal where the ECN counter reads 0.
…om frames Telemetry now records, per device, the NIC's IEEE DCB settings (PFC-enabled priorities and the app table, read like `dcb pfc/app show` with no root) and the traffic class / SL RCCL's sender data QPs use. The fabric report decides RCCL's priority from those settings, checks it against per-priority bytes, and says UNKNOWN instead of guessing when neither is available (AINIC: ionic has no DCB support and no per-priority counters; use --pfc-prio). On NICs that count only pause frames (Thor2), pause is shown as an upper bound, frames x the longest pause one frame can ask for. Measured on AINIC, which counts both, one frame stood for 3 to 53 us, so frames cannot be turned into time and the frames/s threshold is removed. The echo split only blames a host that pushed back when it can explain most of the senders' pause.
- Report: zero ECN marks give 0% even when the slow port packet counter did not move inside the window, instead of "possible CONGESTION-MARKS". - Thor2: read CNP-sent, sequence-error NAK, RNR and out-of-sequence counters under their brcmrdma24 names (np_cnp_pkts, packet_seq_err, rnr_nak_retry_err, out_of_sequence); IB sysfs rows take a second fallback. - accl: open the network window at SendPeerWait, before isend() counts the bytes, so the first send is inside it; a window opened by a receive records that receive's size as rx_bytes_before_window. Collective slots carry a generation, so a late proxy step cannot open the next tenant's window. Captures stop for the run when telemetry reports no NIC. - Telemetry: charge sent bytes per QP to the NIC the chunk went out on; resiliency replays are still not counted. NICs whose received bytes are pooled on a multi-NIC connection are marked rx_bytes_merged. The DCB read moves to first use. - Report: decide the PFC priority per NIC and recompute job-level fields when switching; measure counter refresh lag per counter; keep communicators, algorithms and protocols in separate groups; drop repeated bad reads rather than taking them as a reset; skip records without an execution span; use only the send direction for link sharing on merged-rx NICs. - Install accl_fabric_report.py; probe allocation checks; Thor2 run length and probe-key docs.
dhanavikram
force-pushed
the
users/dhanavikram/rccl-fabric-telemetry-draft
branch
from
November 3, 2026 18:36
5862005 to
186e1ac
Compare
- Stop subtracting a receiver's pause share from the senders' to claim a fabric remainder: the shares are of different ranks' busy time, and backpressure is not a causal bound. Report both sides, also in milliseconds, and say the counters cannot split them. - A NIC with no counters for loss, ECN/CNP, link use or pause no longer comes out CLEAN: the row is INCONCLUSIVE and names what was not measured. - Read the accl end-of-run summary: a file that lost data, or that has no summary because the process did not finish, makes its rows INCONCLUSIVE. - Split receiver-not-ready by where the counter sits: out of buffers is this host (HOST-RNR); RNR NAKs on a sender point at one of its peers (PEER-RNR).
The receive that opens a collective's network window is counted by telemetry before the start snapshot, so it is in no NIC's delta; the plugin records its size as rx_bytes_before_window. The report now shows each NIC's within-window receive bytes (rxwin/coll) and a per-collective total over all NICs (rx/coll) that adds that receive exactly once. The total is a lower bound: one completion can finish several grouped receives and only the first one's size is recorded. Verdicts are unchanged.
Keep out-of-sequence packets, sequence-error NAKs, ACK timeouts and completion errors as separate observations, never summed. Weigh them against endpoint evidence (receive buffers, peer readiness, backpressure, port progress) into HOST_LIKELY, FABRIC_LIKELY, MIXED or INCONCLUSIVE with a confidence that data-quality issues lower. Unmeasured counters are unknown, timeouts alone need healthy endpoints, only a receiver that was not ready counts as a host cause, and every finding the summary headline does not lead with is listed after it. Optional --baseline compares busBW with a healthy run of the same rank count. A counter whose last sample came less than its refresh lag after the collectives may not show their events yet, so its upper bound there is now open. This affects every per-NIC verdict, not only drops: on NICs that refresh about once a second the last size of a run can become INCONCLUSIVE.
Only Thor2's to_retransmits counts retransmits; on AINIC it is the ACK-timeout counter. A rank whose accl records name a NIC that telemetry did not sample is blind for the drop diagnosis and INCONCLUSIVE in its row. Reason text no longer says no pressure was seen when a receiver paused the switch.
This branch has not been deployed
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.
Draft, work in progress. Not ready for review.
Tells whether a slow collective is the fabric's fault or the host's. It correlates per-collective accl profiler timing with RCCL net telemetry NIC counters: rx PFC pause means the switch paused us (fabric), tx pause means we paused the switch (host).
Depends on #13403: this branch sits on #13403's current head, so its commits show here until #13403 merges.
Commits on top of #13403:
fix(rccl): grow accl profiler pools on demand: the fixed 1024-slot proxy-op pool filled when rccl-tests queued ~200 collectives ahead of the GPU (12 ops each on a 2-node ring). Every later collective gotn_proxy_ops0 and no network data, about 54% of records on ruby. Pools now grow in chunks up to a cap. Can be split into its own PR.feat(rccl): attribute slow collectives to fabric or host:accl_fabric_report.pyproduces a text report and a Perfetto trace;tools/net-counter-probereads the same counters standalone.Testing (local, host only):
net_telemetry_test: all passed.ethtool/sysfs logger.