Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ybenvidia wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db57dab
API/UCP/UCT: Add per-endpoint traffic class
ybenvidia Aug 27, 2026
1fcbbb4
discard change
ybenvidia Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ucp/api/ucp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,8 @@ enum ucp_ep_params_field {
/**< Connection request field */
UCP_EP_PARAM_FIELD_CONN_REQUEST = UCS_BIT(6),
UCP_EP_PARAM_FIELD_NAME = UCS_BIT(7), /**< Endpoint name */
UCP_EP_PARAM_FIELD_LOCAL_SOCK_ADDR = UCS_BIT(8) /**< Local socket Address */
UCP_EP_PARAM_FIELD_LOCAL_SOCK_ADDR = UCS_BIT(8), /**< Local socket Address */
UCP_EP_PARAM_FIELD_TRAFFIC_CLASS = UCS_BIT(9) /**< Traffic class */
};


Expand Down
17 changes: 17 additions & 0 deletions src/ucp/api/ucp_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,23 @@ typedef struct ucp_ep_params {
*/
ucs_sock_addr_t local_sockaddr;

/**
Comment thread
ybenvidia marked this conversation as resolved.
* Traffic class for this endpoint, as a full 8-bit Type of Service value:
* a 6-bit Differentiated Services Code Point (DSCP) followed by 2 ECN
* bits. On InfiniBand it is used as the GRH Traffic Class; on RoCEv2 its
* upper 6 bits are used as the DSCP field of the IP header. It overrides
* the transport's default traffic class for this endpoint only, using the
* same format.
*
* Currently implemented by the RC transport over mlx5 devices with DEVX

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* enabled, including the GGA and GDAKI transports which are based on it.
* Transports which do not support it, such as DC, ignore this value and
* keep using their default traffic class.
*
* This setting is optional. To enable it, the corresponding @ref
* UCP_EP_PARAM_FIELD_TRAFFIC_CLASS bit in the field mask must be set.
*/
uint8_t traffic_class;

@shamisp shamisp Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ybenvidia Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 shamisp Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 shasson5 Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ybenvidia Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think also option 2 it's the correct approach

@shamisp shamisp Sep 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 shasson5 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} ucp_ep_params_t;


Expand Down
24 changes: 23 additions & 1 deletion src/uct/api/v2/uct_v2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,10 @@ typedef enum {
UCT_EP_CONNECT_TO_EP_PARAM_FIELD_DEVICE_ADDR_LENGTH = UCS_BIT(0),

/** Endpoint address length */
UCT_EP_CONNECT_TO_EP_PARAM_FIELD_EP_ADDR_LENGTH = UCS_BIT(1)
UCT_EP_CONNECT_TO_EP_PARAM_FIELD_EP_ADDR_LENGTH = UCS_BIT(1),

/** Traffic class */
UCT_EP_CONNECT_TO_EP_PARAM_FIELD_TRAFFIC_CLASS = UCS_BIT(2)
} uct_ep_connect_to_ep_param_field_t;


Expand Down Expand Up @@ -729,6 +732,25 @@ typedef struct uct_ep_connect_to_ep_params {
* default minimal length according to the address buffer contents.
*/
size_t ep_addr_length;

/**
* Traffic class for this endpoint, as a full 8-bit Type of Service value:
* a 6-bit Differentiated Services Code Point (DSCP) followed by 2 ECN
* bits. On InfiniBand it is used as the GRH Traffic Class; on RoCEv2 its
* upper 6 bits are used as the DSCP field of the IP header. It overrides
* the interface's default traffic class for this endpoint only, using the
* same format.
*
* Currently implemented by the RC transport over mlx5 devices with DEVX

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* enabled, including the GGA and GDAKI transports which are based on it.
* Transports which do not support it, such as DC, ignore this value and
* keep using their default traffic class.
*
* This setting is optional. To enable it, the corresponding @ref
* UCT_EP_CONNECT_TO_EP_PARAM_FIELD_TRAFFIC_CLASS bit in the field mask
* must be set.
*/
uint8_t traffic_class;

@shamisp shamisp Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} uct_ep_connect_to_ep_params_t;


Expand Down
Loading