API/UCP/UCT: Add per-endpoint traffic class

[ybenvidia]

What

Add an optional per-endpoint traffic class to the UCP and UCT (v2) APIs:

• UCP: ucp_ep_params_t::ep_traffic_class, enabled by
  UCP_EP_PARAM_FIELD_EP_TRAFFIC_CLASS
• UCT: uct_ep_connect_to_ep_params_t::ep_traffic_class, enabled by
  UCT_EP_CONNECT_TO_EP_PARAM_FIELD_EP_TRAFFIC_CLASS

This PR contains only the API definitions. The implementation follows in a
separate PR (see "Follow-up" below).

Why this cannot be done with the existing configuration

UCX already supports a traffic class, but only as an interface-wide
setting (UCX_IB_TRAFFIC_CLASS), programmed into every QP created on that
interface. It is a single value per process/interface.

The use case requires several different values simultaneously within the same
process and the same interface: endpoints belonging to different logical
groups must be tagged differently so the fabric can arbitrate between their
flows.

Concretely, for collective-communication QoS: a UCC team (≈ an MPI
communicator) is created with a priority, and every UCX endpoint opened for
that team must carry the corresponding traffic class, while endpoints of other
teams — on the same worker and the same device — keep a different one. A
higher-priority collective is then simply run on a higher-priority team.

This is a per-connection property, known only by the caller at connect
time. It cannot be derived from any existing parameter, and an environment
variable cannot express it, since one process needs multiple distinct values at
the same time. Hence the API addition.

The value is deliberately kept opaque and fabric-interpreted, matching what UCX
already does with the interface-wide setting: on RoCEv2 it ends up as the IP
DSCP code point, on IB as the GRH traffic class. No new semantics are
introduced — the same field UCX already programs simply becomes settable per
endpoint. Actual arbitration must still be configured on the fabric
(PFC/ETS/DSCP-to-priority mapping); UCX only carries the value.

Backward compatibility

Wire compatibility — unchanged. Nothing is added to the wire protocol. The
traffic class is not packed into UCX addresses and not exchanged in wireup
messages; it is applied locally, by each side, to its own QP context at connect
time. An old peer and a new peer interoperate with a bit-identical wire format.

Each side applies its own value to its own QP, with no negotiation. If only one
side sets it, that side's outgoing packets carry its traffic class and the peer
keeps the existing default — a partial QoS effect, but no protocol breakage.
This mirrors the existing behavior of the interface-wide setting.

ABI compatibility — preserved. Both new fields are appended at the end
of their respective structures, so no existing field offset changes.

Behavioral compatibility — none by default. The feature is strictly opt-in
through field_mask. When the bit is not set, the code path is unchanged and
falls back to the existing interface-wide value, so current applications are
bit-for-bit unaffected.

Follow-up

Implementation PR: #11618 (UCP endpoint plumbing + RC mlx5 DEVX QP
programming), rebased on top of this one once merged.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests roce on worker 0) · commit a870f668

TL;DR: The "roce on worker 0" job failed because test/apps/test_hooks -n 59 died with SIGILL (core dumped) during the UCM Bistro hook test — a known timing race in UCM's non-atomic code patching (time-based grace window in ucm_bistro_apply_patch_atomic), not anything in PR #11805. Re-run the job; the durable fix is to make the patch quiescence deterministic (or at least longer/retried) instead of a fixed bistro_grace_duration spin.

Full analysis

Summary: contrib/test_jenkins.sh step test_ucm_hooks aborted at iteration 22/30: line 1039: 2154650 Illegal instruction timeout 10 ./test/apps/test_hooks -n 59 → timeout: the monitored command dumped core → exit 1, so the Bash task exited with code 1.

Root cause: SIGILL inside the process while UCM installs mmap hooks by live-patching library code. ucm_bistro_apply_patch_atomic() patches in three steps: (1) atomically write a 2-byte self-jump lock (jmp -2) over the function prologue, (2) spin for ucm_global_opts.bistro_grace_duration wall-clock time hoping any thread already executing past those 2 bytes has left the region, (3) memcpy the remaining patch bytes and then atomically unlock. Step 2 is a pure time-based heuristic with no proof of quiescence. The test intentionally maximizes this race: 59 concurrent threads ((RANDOM % (2*NPROC)) + 1) hammering mmap() while dlopen() triggers hook installation. On a loaded shared CI agent, a thread descheduled mid-prologue can resume after the grace window and execute the half-written instruction stream, which decodes to an invalid opcode → SIGILL. The log corroborates timing-sensitivity: 21 iterations (1–58, 72, 76, 80 threads) passed, and the crash appeared only on one iteration — there is no hang (all iterations complete in ~1.0–1.5 s, no gap >2 min), so this is a genuine race, not a timeout.

Evidence this is unrelated to the PR: the branch ep-traffic-class-api (commit a870f66) touches the UCP/UCT endpoint traffic-class API, while the crash is in src/ucm/bistro code patching, exercised before any UCP endpoint code runs. Git history shows this race is pre-existing and previously band-aided by c77bb48 "UCM/BISTRO: Increase grace time to reduce patch race (#11112)".

Side observation (not the failure, but a latent bug worth filing): earlier in the same log, ./test/apps/test_ucp_config -c 'UCX_MODULES=^ib,rdmacm' printed error: '.../build-test/test/apps/.libs/test_ucp_config' does not exist — the binary was never built. Because test_jenkins.sh:1028 pipes into tee and only checks the subsequent grep, this silent failure is swallowed and the module allow-list check is effectively a no-op.

Implicated commit: unknown for the crash itself (pre-existing race); the most recent related change is c77bb48, shasson5 — "UCM/BISTRO: Increase grace time to reduce patch race (#11112)". PR commit a870f66 is not implicated.

File: src/ucm/bistro/bistro.c:86-97 (time-based grace window + non-atomic memcpy of patch tail); test harness at contrib/test_jenkins.sh:1043

Suggested fix:

1. Immediate: re-run the failed job — this is a flaky, load-dependent race unrelated to PR API/UCP/UCT: Add per-endpoint traffic class #11805. Do not block the traffic-class-API PR on it.
2. Short term in CI: make test_ucm_hooks retry a failing iteration (e.g. 2–3 attempts) before declaring failure, and on failure archive the core file plus test_hooks.log so the faulting RIP/symbol is recoverable instead of only "dumped core".
3. Real fix in UCM: replace the wall-clock spin in ucm_bistro_apply_patch_atomic() with a deterministic quiescence barrier — e.g. membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) (falling back to the current grace spin when unavailable) so all threads are guaranteed to have serialized their instruction pipeline before the tail memcpy. Additionally, prefer the 5-byte near-jump path (ucm_bistro_jmp_near_patch_t, bistro_x86_64.c:345-358) wherever reachable, since it shrinks the unlocked tail to 3 bytes, and consider scaling bistro_grace_duration with load as a stopgap.
4. Separately: add an explicit exit-status check (or set -o pipefail) around the test_ucp_config invocation at contrib/test_jenkins.sh:1028 and fix the missing build target, so the module allow-list test isn't silently skipped.

Related: PR #11112 ("UCM/BISTRO: Increase grace time to reduce patch race", commit c77bb48) — prior mitigation of this same race; no open issue found matching "test_hooks Illegal instruction".

[gleon99]

@shasson5 please review.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests new on worker 0) · commit 168d00c3

TL;DR: The build failed only on the flaky timing-sensitive test_rwlock.lock gtest (8537/8538 passed); it is unrelated to the traffic-class API change in PR #11805 and is already being addressed by PR #11809, so re-run CI / rebase once that fix lands.

Full analysis

Summary: make test in build-test/test/gtest exited with Error 1 because exactly one test — test_rwlock.lock — failed, out of 8538 tests run.

Root cause: Evidence from the log: [ PASSED ] 8537 tests. / [ FAILED ] 1 test, listed below: test_rwlock.lock, where TypeParam = and GetParam() =, then make: *** [Makefile:4713: test] Error 1. Nothing else in the ~3697 s run failed, and there is no hang (log timestamps are continuous, longest inter-line gap is ~11 s during cma/uct_p2p_rma_test.put_zcopy). test_rwlock.lock is a pure ucs_rw_spinlock_t unit test (see src/ucs/type/rwlock.h, a self-contained header with no UCP/UCT endpoint dependencies) that synchronizes reader/writer threads with fixed sleeps. On a loaded CI worker those sleeps race — the same suite shows test_rwlock.memory_barriers at 30162 ms, the 14th-longest test in the run, confirming this suite is heavily timing/scheduling dependent. The PR under test (ep-traffic-class-api, commit 168d00c) touches the endpoint traffic-class API and cannot affect the UCS rwlock primitive, so this is a pre-existing flake, not a regression from the PR. The concurrent open PR #11809 ("GTEST/UCS: Increase rwlock test sleep to avoid CI races") confirms this is a known CI race.

Implicated commit: Not the PR commit. The rwlock primitive/test originates from 8bbe776 "UCS: Introduce lightweight rwlock" (Artemy Kovalyov) — the only commit touching src/ucs/type/rwlock.h. No commit in ep-traffic-class-api is implicated.

File: test/gtest/ucs/test_rwlock.cc (the test_rwlock.lock case); primitive under test: src/ucs/type/rwlock.h:56-135

Suggested fix: Treat this as an unrelated flake for PR #11805 — retrigger the Azure "Tests new on worker 0" job, and rebase the branch after PR #11809 (which increases the rwlock test's sleep to remove the CI race) is merged. Longer term, make test_rwlock.lock deterministic instead of sleep-tuned: replace the fixed sleeps with explicit synchronization (a pthread_barrier_t/condition-variable rendezvous, or a polling loop on an atomic flag with a generous timeout) so the assertion no longer depends on thread scheduling latency on a busy worker. Do not raise any job time limit — the run completed and the only failure is this assertion.

Related: #11809 (GTEST/UCS: Increase rwlock test sleep to avoid CI races); failing build PR #11805

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests roce on worker 3) · commit 5a326b4d

TL;DR: dcx/test_ucp_am_nbx.rx_persistent_data/0 <dc_x> never received the 1‑byte active message on the RoCE worker (10 s wait_for_flag timeout), and because the test uses EXPECT_TRUE instead of ASSERT_TRUE it then dereferenced the NULL rx_data, segfaulting the whole gtest binary and killing the job. Fix the test to assert/return on the NULL pointer, and debug why the DC (dc_x) AM is silently dropped on RoCE with the PR's per‑endpoint traffic‑class (DSCP) changes.

Full analysis

Summary: UCP AM test over dc_x on RoCE timed out waiting for the persistent-data receive callback, then crashed with SIGSEGV (make: *** [Makefile:4713: test] Segmentation fault (core dumped)), aborting the entire gtest run.

Root cause:

• Log evidence: test starts at 09:04:07.555, fails at 09:04:17.864 — exactly the ~10 s wait_for_flag polling timeout, i.e. am_data_hold_cb was never invoked, so the 1‑byte eager AM sent over dc_x was never delivered on the receiver. No UCX error/diag was printed, which is the signature of a silently dropped/misrouted IB packet rather than a reported transport failure. (The 09:04:17 → 09:06:11 gap is only libucs backtrace symbolization after the signal, not a hang.)
• Crash amplifier: test/gtest/ucp/test_ucp_am.cc:894 uses EXPECT_TRUE(rx_data != NULL) (non-fatal). Execution continues to line 895, EXPECT_EQ(data, *reinterpret_cast<char*>(rx_data)), dereferencing NULL — matching the backtrace frames testing::internal::CmpHelperEQ<char, char> → ucs::test_base::run() test.cc:463 and "Caught signal 11 … at address (nil)". This turns a single test failure into a crashed test binary and a red build.
• Attribution: this is the "roce" worker and the only failing case is the DC (dc_x) UCP data path; PR API/UCP/UCT: Add per-endpoint traffic class #11805 ("API/UCP/UCT: Add per-endpoint traffic class", branch ep-traffic-class-api, successor of UCP/UCT/IB: Add per-endpoint traffic class (DSCP) for RC/RoCE (QOS) #11618 "per-endpoint traffic class (DSCP) for RC/RoCE") changes exactly the AH/QP traffic-class attributes used when building RC/RoCE and DC address handles. A per-EP tclass/DSCP value leaking into the DCI/AH creation path (instead of the iface-level RoCE TC autodetection added in e7b7c866 "UCT/IB: Use global traffic class for RoCEv2 with auto configuration") would produce exactly this symptom: connection established, sends posted, packets never arriving. Note the base-branch source does not contain the traffic-class code, so this part is inferred from the failure locality, not read from the diff.

Implicated commit: [REDACTED:Hex High Entropy String] (PR #11805, branch ep-traffic-class-api) for the delivery failure; the NULL-deref crash pattern predates it (test/gtest/ucp/test_ucp_am.cc, last touched by b6b8531, tomerg-nvidia).

File: test/gtest/ucp/test_ucp_am.cc:893-895 (crash site); per-EP traffic-class/AH-attribute code added by PR #11805 in src/uct/ib/base/ib_iface.c / src/uct/ib/dc/dc_mlx5_ep.c (functional cause)

Suggested fix:

1. Make the test fail safely instead of crashing the suite:

   wait_for_flag(&rx_data);
   ASSERT_TRUE(rx_data != NULL);          /* was EXPECT_TRUE */
   EXPECT_EQ(data, *reinterpret_cast<char*>(rx_data));

   (or if (rx_data == NULL) { request_wait(sptr); return; }) — this alone stops one flaky/failing AM test from segfaulting the whole make test.
2. For the real failure, verify in the PR that when no traffic class is requested on an endpoint, the DC/RoCE path still uses the iface-level default (auto-detected RoCEv2 TC / UCX_IB_TRAFFIC_CLASS) rather than overwriting ah_attr.grh.traffic_class/QP tclass with 0 or an uninitialized value; check the dc_mlx5 DCI AH creation path specifically, since it goes through the same uct_ib_iface_fill_ah_attr_* helpers as RC but is not covered by the RC-focused changes.
3. Re-run with --gtest_filter='dcx/test_ucp_am_nbx.rx_persistent_data*' and UCX_LOG_LEVEL=debug on the RoCE node to confirm reproducibility and dump the effective tclass/DSCP for the DC endpoint; compare against a build of the merge base to rule out an environment flake.

Related: #11805, #11618, commit e7b7c86 "UCT/IB: Use global traffic class for RoCEv2 with auto configuration (#11105)"

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id adb86e45-4ec7-4165-bde7-54a673aee45f in the triage console for the audit trail.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

The new field and mask bit have no consumer in this PR — no transport reads ep_traffic_class. The doc states this is "currently implemented by the RC transport over mlx5 devices with DEVX enabled...", but the diff and PR head contain no implementation. If the API is split from the implementation on purpose, please reword the doc so it doesn't claim it is "currently implemented" until the RC mlx5 support lands, otherwise users will set it and silently get the iface default.

[svc-nvidia-pr-review]

The new field and mask bit have no consumer in this PR — no transport reads ep_traffic_class. The doc states this is "currently implemented by the RC transport over mlx5 devices with DEVX enabled...", but the diff and PR head contain no implementation. If the API is split from the implementation on purpose, please reword the doc so it doesn't claim it is "currently implemented" until the RC mlx5 support lands.

[svc-nvidia-pr-review]

Coverage gap: No test exercises the new API contract (e.g., a UCP ep-create path that sets UCP_EP_PARAM_FIELD_EP_TRAFFIC_CLASS, or a UCT connect_to_ep path). Per REVIEW.md, new APIs should usually include a test that exercises the contract. If this PR is intentionally API-only ahead of the implementation, that intent should be stated in the PR description and the "currently implemented" wording removed until the implementation lands.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

Residual coverage note (already raised in the prior review, not blocking): there is no test exercising the new field-mask paths and no implementation reads the new bits. If this is intentionally API-only ahead of the implementation, the "Currently implemented by the RC transport..." wording in both ucp_def.h and uct_v2.h is misleading and should be deferred until the implementation lands.

[shasson5]

@ybenvidia please avoid force-pushing during code review process

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

Misleading "Currently implemented by the RC transport over mlx5 devices with DEVX enabled..." wording. No code reads traffic_class from the params struct, so the documentation claims a working implementation that does not exist in this PR. This wording should be deferred until the implementation lands, or the PR description should state the API-only intent.

[svc-nvidia-pr-review]

Misleading "Currently implemented by the RC transport over mlx5 devices with DEVX enabled..." wording. No code reads traffic_class from the params struct, so the documentation claims a working implementation that does not exist in this PR. This wording should be deferred until the implementation lands, or the PR description should state the API-only intent.

[svc-nvidia-pr-review]

No test exercises the new API contract — no UCP ep-create or UCT connect_to_ep path sets the new field-mask bits (UCP_EP_PARAM_FIELD_TRAFFIC_CLASS / UCT_EP_CONNECT_TO_EP_PARAM_FIELD_TRAFFIC_CLASS). New APIs should usually include a test that exercises the contract.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Codestyle commit title) · commit 1fcbbb4b

TL;DR: This is not a code/test failure — the "commit title" codestyle gate rejected the PR because one of its commits is titled discard change, which lacks the required Module: Capitalized description prefix. Reword or squash that commit (e.g. fold it into API/UCP/UCT: Add per-endpoint traffic class) and force-push.

Full analysis

Summary: The Azure Pipelines "Codestyle / commit title" job (build 133025) exited with code 1 during its Bash step after validating the PR's commit titles.

Root cause: The commit-title linter iterates over the commits in PR #11805 and requires each title to match UCX's convention <MODULE>[/<SUBMODULE>]: <Capitalized description>. The log shows exactly two evaluations:

• Bad commit title: 'discard change' — no module prefix, not capitalized → rejected
• Good commit title: 'API/UCP/UCT: Add per-endpoint traffic class' → accepted

The Bash task then failed (##[error]Bash exited with code '1'). Nothing was compiled or tested; the merge commit 04c9d0081 (merge of 1fcbbb4b into 45a9372265) checked out fine, and the checkout/fetch output before the error is entirely normal git progress with no timing gaps. So this is a deterministic lint failure, not flakiness, an infra issue, or a hang.

Implicated commit: [REDACTED:Hex High Entropy String] (branch ep-traffic-class-api) is the PR head; the offending title discard change belongs to a commit in that branch's PR range. Not a regression in the CI scripts — the checker behaved as designed.

File: No source file is at fault. The failing step is the "commit title" Bash task of the Codestyle stage in the Azure pipeline definition under buildlib/ (exact path not resolvable from this checkout; the tool could not read buildlib/tools/commit_title.sh, buildlib/commit-title.sh, buildlib/pr/check-commit-format.yml, or buildlib/az-check-commit-title.yml).

Suggested fix: Rewrite the branch history to remove the non-conforming title:

git rebase -i [REDACTED:Hex High Entropy String]
# mark the 'discard change' commit as 'fixup' (or 'reword' it to e.g.
#   'API/UCP/UCT: Fix traffic class ...')
git push --force-with-lease origin ep-traffic-class-api

Simplest option, since discard change looks like a scratch/WIP commit: fixup/squash it into API/UCP/UCT: Add per-endpoint traffic class so the PR carries a single well-formed title. Re-running CI without changing history will fail identically.

Related: none (searches for prior "Bad commit title" reports returned only unrelated PRs: #11838, #11796, #11480, #11218, #2724)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id bc63cab3-69db-4de4-843f-cf2f0eecc7e4 in the triage console for the audit trail.

[shasson5]

LGTM
@gleon99 can review?

[shamisp]

This API change is tricky. We are trying to expose transport specific (TL) concept into UCP and we end up calling out explicitly RC/IB/ROCE. Would it make sense to define HIGH/LOW/etc. level and underneath implement relevant mapping ?

[ybenvidia]

@shamisp You're right, and I checked with Feroz Zahid on the QoS side — he says the same thing: DSCP is not what an application should be setting. So I'll rework the UCP side.

Proposed direction:

• UCP: an abstract priority level instead of a traffic class — a small integer with 0 = highest priority. Feroz suggests reserving ~5 bits so additional levels can be given meaning later without changing the API (in practice only "high vs the rest" is used today, but this keeps the door open).

• UCT: keep traffic_class as it is in this PR, since you said this layer is the right place for it.

• The level-to-value mapping must be site-configurable, not hardcoded.

One design question before I code it: should UCP resolve the level into a traffic class — with the transport publishing its mapping through the iface attributes — or should the level be passed down to UCT and resolved there? The first keeps the UCT API exactly as you approved it; the second puts the mapping where the fabric config already lives.

Also @shasson5 what do you think ?

[shamisp]

You have two options in general:

1. UCT exposing API that let's you query support levels and UCP doing the mapping
2. Push this all the way down to UCT and UCT would do the level to value translation.

If we anticipate protocols orchestrating various priorities, I think (1) is correct path. If protocols are not expected to operate on QoS level, then (2) better choice.

[shasson5]

in our case protocols are not expected to deal with QoS (no special lanes/transport/protocols selection), so option 2 is the correct approach.
also I think maybe worth moving the new UCT level field to UCT EP creation API (uct_ep_params_t).
@shamisp WDYT?

[ybenvidia]

I think also option 2 it's the correct approach

[shamisp]

Move it to Ep creation instead of EP connect ? probably. Can single EP support multiple QoS levels ? This is a scalability question. Creating extra UCT EP and management those is not free.

[shasson5]

@ybenvidia do you have a requirement/design document that describes the specific use case/scenario that we need to support?

[shamisp]

On UCT this makes much more sense but for UCP as I mentioned earlier it is not a good abstraction.