Skip to content

Add auto-detect for RoCE v2 GID for mlx5 NICs in MlxDevice#4304

Open
mkuchnik wants to merge 2 commits into
meta-pytorch:mainfrom
mkuchnik:mkuchnik/mlx-roce-v2-gid-autodetect
Open

Add auto-detect for RoCE v2 GID for mlx5 NICs in MlxDevice#4304
mkuchnik wants to merge 2 commits into
meta-pytorch:mainfrom
mkuchnik:mkuchnik/mlx-roce-v2-gid-autodetect

Conversation

@mkuchnik

Copy link
Copy Markdown
Contributor

rdma-core's default gid_index (3) is not the routable GID on RoCE v2 fabrics, and mlx5 does no auto-selection, so QPs come up on an unroutable GID. This diff adds autoselection logic for MlxDevice:

  1. MONARCH_IB_GID_INDEX, if set (as an override);
  2. else auto-detect the first routable RoCE v2 GID from sysfs (gid_attrs/types/ == "RoCE v2" and gids/ not link-local / all-zero);
  3. else fall back to gid_index 3 (prior InfiniBand-mode NICs behavior).

The result is cached process-wide.
Tested on a GB300 node with ConnectX-8 SuperNIC, where the routable GID is index 7.

rdma-core's default gid_index (3) is not the routable GID on RoCE v2 fabrics,
and mlx5 does no auto-selection, so QPs come up on an unroutable GID.
This diff adds autoselection logic for MlxDevice:
  1. MONARCH_IB_GID_INDEX, if set (as an override);
  2. else auto-detect the first routable RoCE v2 GID from sysfs
     (gid_attrs/types/<i> == "RoCE v2" and gids/<i> not link-local / all-zero);
  3. else fall back to gid_index 3 (prior InfiniBand-mode NICs behavior).

The result is cached process-wide.
Tested on a GB300 node with ConnectX-8 SuperNIC, where the routable GID is index 7.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jun 26, 2026
@mkuchnik mkuchnik requested a review from samlurye June 26, 2026 19:59
Comment thread monarch_rdma/src/backend/ibverbs/mlx_device.rs Outdated
Comment thread monarch_rdma/src/backend/ibverbs/roce_gid.rs Outdated

/// `MONARCH_IB_GID_INDEX` parsed as a `u8`, if set and valid.
fn env_gid_index() -> Option<u8> {
std::env::var("MONARCH_IB_GID_INDEX")

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.

The canonical way to define an env var option in monarch is using a hyperactor config attribute; the options for RDMA are defined here. Please call it RDMA_IBV_DEFAULT_GID_INDEX, and incorporate it into the code base by setting the value in IbvConfig's default implementation.

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.

Take a look. The downside of spreading autoconfig between 1) env 2) autoconfig 3) default is there is an unclear ordering of priority. For example, autoconfig is applied after env and default, so the priority order seems 2, 1, 3, which may be unexpected.

Comment thread monarch_rdma/src/backend/ibverbs/roce_gid.rs Outdated
@mkuchnik mkuchnik force-pushed the mkuchnik/mlx-roce-v2-gid-autodetect branch from 84bed5c to 8119cdf Compare June 29, 2026 23:07
Addresses review of the auto-detect commit. Moves the GID-selection logic out
of the standalone roce_gid module and onto primitives::Gid.

The default is RDMA_IBV_DEFAULT_GID_INDEX (env MONARCH_RDMA_IBV_DEFAULT_GID_INDEX).

Tested on a GB300 node with ConnectX-8 SuperNIC, where the routable GID is index 7.
@mkuchnik mkuchnik force-pushed the mkuchnik/mlx-roce-v2-gid-autodetect branch from 8119cdf to 47ceac0 Compare June 29, 2026 23:36
@mkuchnik mkuchnik requested a review from samlurye June 30, 2026 00:48
Some("MONARCH_RDMA_IBV_DEFAULT_GID_INDEX".to_string()),
Some("rdma_ibv_default_gid_index".to_string()),
))
pub attr RDMA_IBV_DEFAULT_GID_INDEX: Option<u8> = None;

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.

Let's make this non-optional with a default of 0. The comment shouldn't mention mlx5 or any specific hardware.

///
/// Reads the kernel's GID table from sysfs
/// (`/sys/class/infiniband/<dev>/ports/<p>/gid_attrs/types/<i>` and
/// `gids/<i>`) rather than `ibv_query_gid*`: on some mlx5 / RoCE stacks the

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.

A few things:

  1. Comments in non-hardware-specific files shouldn't be mentioning specific hardware that uses it.
  2. Are you actually in practice running into cases where ibv_query_gid returns a bad value?
  3. If device_info doesn't contain the right thing, then we should update IbvDeviceInfo so that it does contain the right thing. Instead of having each IbvPort contain only a single gid, let's give it the full list of gids, read from sysfs (filtering out non-roce-v2 gids), and fill it in inside query_device_info. Then Gid::index_of can just read from IbvDeviceInfo without having to go through sysfs or any additional ibv_query_gid calls.
  4. Please tell the LLM to be more concise in its comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants