Repository navigation
fix(rccl): correct IB-CAST network telemetry counters on bnxt_re (AICOMNET-432) - #13403
Merged
Merged
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. |
dhanavikram
changed the title
dhanavikram
force-pushed
the
users/dhanavikram/aicomnet-432-net-counters
branch
from
October 13, 2026 20:22
4f4005f to
d58bc5e
Compare
dhanavikram
marked this pull request as ready for review
October 14, 2026 20:42
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/src/transport/net_ib_cast/connect.cc:1457(Low) QP-sharing secondary with nqps > primaryNqps gives two telemetry slots to one shared ibv_qp; completions all land on the lower slotprojects/rccl/src/transport/net_telemetry.cc:544(Nit) Thor2 rx/tx_errors and rx/tx_discards rows list the old name first, against the table's 'current key first' commentprojects/rccl/src/transport/net_telemetry.cc:655(Nit) rcclTelemetryEthtoolResolve: wrapped if condition with unbraced body on its own line; fits on one line at 105 colsprojects/rccl/src/transport/net_ib_cast/connect.cc:1445(Nit) Telemetry setup comment names IbCastQpCreate(), but the block now also covers QP-sharing secondaries that never call itprojects/rccl/test/net_telemetry_test.cpp:938(Nit) testEthtoolFallbackOrder comment describes an rx_pause_frames/rx_pfc_frames fallback pair that no table has after this PR
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
This was referenced Oct 20, 2026
nikitaxgusev
approved these changes
Oct 22, 2026
Contributor
There was a problem hiding this comment.
LGTM. All my review points are addressed: the counter wrap is now scoped to the bnxt_re byte/packet deltas via wrap_bits, every other counter returns -1 on a decrease, the NULL-key guard lives in rcclTelemetrySetupQps with a unit test, and num_qp_untracked now also warns on stderr. Approving from my side.
VadimKutovoi
approved these changes
Oct 28, 2026
VadimKutovoi
left a comment
VadimKutovoi
left a comment
Contributor
There was a problem hiding this comment.
Looks good, thanks!
dhanavikram
force-pushed
the
users/dhanavikram/aicomnet-432-net-counters
branch
from
October 28, 2026 14:39
9bfc21c to
4be8436
Compare
Contributor
pr-rocm-systems-13403-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. |
Contributor
pr-rocm-systems-13403-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. |
ddurnov
approved these changes
Nov 2, 2026
dhanavikram
force-pushed
the
users/dhanavikram/aicomnet-432-net-counters
branch
from
November 2, 2026 16:36
4be8436 to
d7ea0f7
Compare
Contributor
pr-rocm-systems-13403-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13403-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13403-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13403-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
…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.
dhanavikram
force-pushed
the
users/dhanavikram/aicomnet-432-net-counters
branch
from
November 3, 2026 15:31
24d9147 to
c217c72
Compare
Contributor
pr-rocm-systems-13403-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13403-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
Motivation
On bnxt_re (Thor2), IB-CAST telemetry reports -1 for 11 counters (incl. CNP) and 0 for PFC, because counter names changed across driver versions and the PFC fields read global pause.
Technical Details
rx/tx_pfc_frames.transportis"none"when no device registered.Issue Tracking
JIRA ID: AICOMNET-432
Test Plan
net_telemetry_test: ethtool fallback order, thor2 table against a fake bnxt_re sysfs tree.NCCL_NET=IB-CAST: all_reduce, alltoall, pre-posted receives,NCCL_IB_HCAexclude. Telemetry JSON compared against sysfs/ethtool deltas for the same run.Test Result
Unit tests pass (also under ASan/UBSan). End to end: 0 mismatches in every run, including nonzero PFC. QP sharing could not be run end to end: it fails with
ibv_post_sendENOMEM on Thor2 with or without this change.Submission Checklist