Skip to content
21 changes: 21 additions & 0 deletions src/ucp/core/ucp_context.c
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ static const char *ucp_fence_modes[] = {
[UCP_FENCE_MODE_LAST] = NULL
};

static const char *ucp_gpu_nic_assignment_modes[] = {
[UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO] = "auto",
[UCP_GPU_NIC_ASSIGNMENT_MODE_OFF] = "off",
[UCP_GPU_NIC_ASSIGNMENT_MODE_FLIP] = "flip",

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.

for my understanding: can you pls remind me when/why the flip policy is needed?

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.

It's because only one of the NICs on a CX9 board has SSD memory, so the goal is to have each GPU assigned to one with and one without SSD memory access.

[UCP_GPU_NIC_ASSIGNMENT_MODE_ROUND_ROBIN] = "round_robin",
[UCP_GPU_NIC_ASSIGNMENT_MODE_SHARED] = "shared",
[UCP_GPU_NIC_ASSIGNMENT_MODE_LAST] = NULL
};

static const char *ucp_rndv_modes[] = {
[UCP_RNDV_MODE_AUTO] = "auto",
[UCP_RNDV_MODE_GET_ZCOPY] = "get_zcopy",
Expand Down Expand Up @@ -162,6 +171,7 @@ const char *ucp_extra_op_attr_flags_names[] = {
static UCS_CONFIG_DEFINE_ARRAY(memunit_sizes, sizeof(size_t),
UCS_CONFIG_TYPE_MEMUNITS);

/* clang-format off */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor: the clang-format off/on markers around the whole config table (and the matching clang-format on at L685) are unrelated to this change, maybe move them to a separate PR.

Rationale: only src/uct/ib/base/ib_device.c uses these markers today, and the two lines wrap ~500 unrelated lines; REVIEW.md asks feature PRs not to carry unrelated changes.

static ucs_config_field_t ucp_context_config_table[] = {
{"SELECT_DISTANCE_MD", "cuda_cpy",
"MD whose distance is queried when evaluating transport selection score",
Expand Down Expand Up @@ -615,6 +625,16 @@ static ucs_config_field_t ucp_context_config_table[] = {
ucs_offsetof(ucp_context_config_t, connect_all_to_all),
UCS_CONFIG_TYPE_BOOL},

{"GPU_NIC_ASSIGNMENT_MODE", "auto",
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
"Select how NICs are assigned to GPUs.\n"
" - auto : enable automatically on supported hardware, otherwise disable.\n"

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.

Suggested change
" - auto : enable automatically on supported hardware, otherwise disable.\n"
" - auto : use flip if on supported hardware, otherwise disable.\n"

I think it should mention what policy will be used, no?

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.

In general the policy can be adapted automatically according to whatever hw is detected, so I decided to not be too specific here.

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 "not shared" wording is hard to follow for users reading ucx_info -f, and two of the new help lines are 86/87 columns, exceeding the 80-column limit in docs/CodeStyle.md. Suggested rewording:

Suggested change
" - auto : enable automatically on supported hardware, otherwise disable.\n"
" - auto : enable on supported hardware, otherwise disable.\n"
" - off : disable assignment, fall back to free lane selection.\n"
" - flip : assign each NIC to a single GPU, forward then backward.\n"
" - round_robin : assign each NIC to a single GPU, in ascending order.\n"
" - shared : assign all NICs of a group to all GPUs of the group.",

" - off : disable GPU-to-NIC assignment, fallback to free lane selection.\n"
" - flip : require the not shared flip assignment policy.\n"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor: "require the not shared flip assignment policy" is hard to parse; pls describe what each policy does.

Suggested change
" - flip : require the not shared flip assignment policy.\n"
" - flip : assign each NIC to a single GPU, alternating direction.\n"
" - round_robin : assign each NIC to a single GPU in round-robin order.\n"
" - shared : assign all NICs of a group to every GPU in the group.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

help text wording — minor: "require the not shared ... policy" is hard to parse for users. Suggested:

 " - flip        : force the exclusive flip assignment policy.\n"
 " - round_robin : force the exclusive round-robin assignment policy.\n"
 " - shared      : force the shared assignment policy.",

" - round_robin : require the not shared round-robin assignment policy.\n"
" - shared : require the shared assignment policy.",
ucs_offsetof(ucp_context_config_t, gpu_nic_assignment_mode),
UCS_CONFIG_TYPE_ENUM(ucp_gpu_nic_assignment_modes)},

{"SINGLE_NET_DEVICE", "n",
"Restrict each protocol's lanes to one network device.\n"
"The device is picked among the closest eligible network devices using \n"
Expand Down Expand Up @@ -650,6 +670,7 @@ static ucs_config_field_t ucp_context_config_table[] = {

{NULL}
};
/* clang-format on */

static ucs_config_field_t ucp_config_table[] = {
{"NET_DEVICES", UCP_RSC_CONFIG_ALL,
Expand Down
2 changes: 2 additions & 0 deletions src/ucp/core/ucp_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ typedef struct ucp_context_config {
/** Extend endpoint lanes connections of each local device to all remote
* devices */
int connect_all_to_all;
/** GPU-to-NIC assignment mode */
ucp_gpu_nic_assignment_mode_t gpu_nic_assignment_mode;
/** Restrict lanes to one network device per protocol */
int proto_use_single_net_device;
/** Max HCAs for GPU memory registration: auto=closest, N=limit, inf=all */
Expand Down
78 changes: 71 additions & 7 deletions src/ucp/core/ucp_gpu_nic_assignment.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,35 @@ typedef struct {
} ucp_nics_string_buffers_t;


int ucp_gpu_nic_assignment_policy_resolve(

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.

maybe we drop ucp_gpu_nic_assignment_policy_t and reuse ucp_gpu_nic_assignment_mode_t (or add missing state to existing ucp_gpu_nic_assignment_policy_t)? ucp_rndv_mode_t keeps auto in the same enum, so we would not need a second enum and a mapping between them, it could be:

  • move the policy doc comments onto the mode values in ucp_types.h, and delete the policy enum
  • pass the mode to ucp_gpu_nic_assignment_build(), add_group() and get_gpu_idx()
  • resolve auto to flip/off in ucp_context_gpu_nic_assignment_init(), where the cpu model and the mode names table are already available
  • then ucp_gpu_nic_assignment_policy_resolve() and the mode_resolution test are not needed, and the header does not need ucp_types.h and ucs/arch/cpu.h
  • pls also keep the "not supported on %s architecture" debug from UCS/TOPO: Initialize GPU-NIC assignment object in UCP context #11976 there, and print the resolved mode when it is enabled

the streamlined check in ucp_context_gpu_nic_assignment_init() would be like below and this leaves the PR with just the shared policy, the config knob and the shared tests.

    ucp_gpu_nic_assignment_mode_t mode =
            context->config.ext.gpu_nic_assignment_mode;
    ...

    if (mode == UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO) {
        /* TODO: Improve Vera Rubin detection by checking NICs/GPUs models. */
        mode = (ucs_arch_get_cpu_model() == UCS_CPU_MODEL_NVIDIA_VERA) ?
                       UCP_GPU_NIC_ASSIGNMENT_MODE_FLIP :
                       UCP_GPU_NIC_ASSIGNMENT_MODE_OFF;
    }

    if (mode == UCP_GPU_NIC_ASSIGNMENT_MODE_OFF) {
        ucs_debug("gpu-nic assignment is disabled, cpu model %s",
                  ucs_cpu_model_name());
        return UCS_OK;
    }

    ucs_debug("gpu-nic assignment mode %s, cpu model %s",
              ucp_gpu_nic_assignment_modes[mode], ucs_cpu_model_name());

    /* ... build groups, allocate assignment ... */

    status = ucp_gpu_nic_assignment_build(&groups, mode, assignment);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pls add a gtest for ucp_gpu_nic_assignment_policy_resolve() (auto on/off, off, and the forced modes) — the cpu_model argument is injectable exactly for that, and today nothing exercises the mode→policy mapping.

Rationale: the whole user-visible behavior added by this PR is the mode→policy resolution; test_ucp_gpu_nic_assignment.cc still only calls ucp_gpu_nic_assignment_build() with an explicit policy, so auto/off and the enum-to-policy mapping have no coverage at all. This is cheap to test since the function is pure.

ucp_gpu_nic_assignment_mode_t mode, ucs_cpu_model_t cpu_model,
ucp_gpu_nic_assignment_policy_t *policy_p)
{
ucs_assert(policy_p != NULL);

switch (mode) {
case UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO:
if (cpu_model != UCS_CPU_MODEL_NVIDIA_VERA) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ucs_topo_build_groups() already refuses to build groups when the CPU model is not Vera, so the check is duplicated here, and flip/round_robin/shared can never take effect on any other hardware even though the help text says they are "required". Can we expose a topo helper for "groups supported" and use it here instead of re-checking the cpu model in UCP?

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 happens implicitly with the other policies too, no? Is this really needed if the topology groups aren't being created on vera anyway?

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.

When #11976 is merged I would remove the other check and depend only on this function to check if groups and assignments should be built

return 0;
}
*policy_p = UCP_GPU_NIC_ASSIGNMENT_POLICY_FLIP;
return 1;
case UCP_GPU_NIC_ASSIGNMENT_MODE_OFF:
return 0;
case UCP_GPU_NIC_ASSIGNMENT_MODE_FLIP:
*policy_p = UCP_GPU_NIC_ASSIGNMENT_POLICY_FLIP;
return 1;
case UCP_GPU_NIC_ASSIGNMENT_MODE_ROUND_ROBIN:
*policy_p = UCP_GPU_NIC_ASSIGNMENT_POLICY_ROUND_ROBIN;
return 1;
case UCP_GPU_NIC_ASSIGNMENT_MODE_SHARED:
*policy_p = UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED;
return 1;
default:
ucs_fatal("invalid gpu-nic assignment mode %d", (int)mode);
}
}

static size_t
ucp_gpu_nic_assignment_get_gpu_idx(ucp_gpu_nic_assignment_policy_t policy,
size_t num_gpus, size_t nic_idx)
Expand Down Expand Up @@ -327,16 +356,39 @@ ucp_gpu_nic_assignment_init(ucp_gpu_nic_assignment_t *assignment,
}

static void
ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment,
const ucs_topo_group_t *group,
ucp_gpu_nic_assignment_policy_t policy)
ucp_gpu_nic_assignment_add_group_shared(ucp_gpu_nic_assignment_t *assignment,

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.

can we merge add_group_shared() and add_group_not_shared() into a single function either one of the two loop order below so that we have one assignment area?

static void
ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment,
                                 const ucs_topo_group_t *group,
                                 ucp_gpu_nic_assignment_policy_t policy)
{
    size_t num_gpus = ucs_array_length(&group->gpus);
    ucp_gpu_nic_sys_dev_bitmap_t *nic_sys_dev_bitmap;
    const ucs_topo_group_element_t *gpu, *nic;
    size_t gpu_idx, nic_idx;
    ucs_array_for_each_index(gpu, gpu_idx, &group->gpus) {
        ucs_assert((gpu->num_sys_devs > 0) &&
                   (gpu->sys_devs[0] != UCS_SYS_DEVICE_ID_UNKNOWN));
        nic_sys_dev_bitmap = ucs_const_cast(
                ucp_gpu_nic_sys_dev_bitmap_t*,
                ucp_gpu_nic_assignment_lookup(assignment, gpu->sys_devs[0]));
        ucs_assert(nic_sys_dev_bitmap != NULL);
        ucs_array_for_each_index(nic, nic_idx, &group->nics) {
            if ((policy != UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED) &&
                (ucp_gpu_nic_assignment_get_gpu_idx(policy, num_gpus,
                                                    nic_idx) != gpu_idx)) {
                continue;
            }
            ucp_gpu_nic_bitmap_add_nic(nic_sys_dev_bitmap, nic);
        }
    }
}

iterating the nics first in the shared case sets the same bits, so the tests should not change:

static void
ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment,
                                 const ucs_topo_group_t *group,
                                 ucp_gpu_nic_assignment_mode_t mode)
{
    size_t num_gpus = ucs_array_length(&group->gpus);
    const ucs_topo_group_element_t *gpu, *nic;
    size_t nic_idx, gpu_idx;

    ucs_array_for_each_index(nic, nic_idx, &group->nics) {
        if (mode == UCP_GPU_NIC_ASSIGNMENT_MODE_SHARED) {
            /* Assign the NIC to every GPU in the group. */
            ucs_array_for_each(gpu, &group->gpus) {
                ucp_gpu_nic_assignment_add_nic_to_gpu(assignment, gpu, nic);
            }
        } else {
            gpu_idx = ucp_gpu_nic_assignment_get_gpu_idx(mode, num_gpus,
                                                         nic_idx);
            gpu     = &ucs_array_elem(&group->gpus, gpu_idx);
            ucp_gpu_nic_assignment_add_nic_to_gpu(assignment, gpu, nic);
        }
    }
}

const ucs_topo_group_t *group)
{
ucp_gpu_nic_sys_dev_bitmap_t *nic_sys_dev_bitmap;
const ucs_topo_group_element_t *gpu, *nic;

ucs_array_for_each(gpu, &group->gpus) {
ucs_assert((gpu->num_sys_devs > 0) &&
(gpu->sys_devs[0] != UCS_SYS_DEVICE_ID_UNKNOWN));

nic_sys_dev_bitmap = ucs_const_cast(
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved

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 gpu bitmap lookup and asserts here are a copy of the block in ucp_gpu_nic_assignment_add_group_not_shared(). can we extract ucp_gpu_nic_assignment_add_nic_to_gpu(assignment, gpu, nic) and call it from both? then shared is just an inner ucs_array_for_each(gpu, &group->gpus) inside the existing NIC loop and the extra dispatch function is not needed. AGENTS.md: "Do not duplicate code or similar implementation patterns; use helper functions".

ucp_gpu_nic_sys_dev_bitmap_t*,
ucp_gpu_nic_assignment_lookup(assignment, gpu->sys_devs[0]));
ucs_assert(nic_sys_dev_bitmap != NULL);

/* Assign all NICs in the group to the GPU. */
ucs_array_for_each(nic, &group->nics) {
ucp_gpu_nic_bitmap_add_nic(nic_sys_dev_bitmap, nic);
}
}
}

static void ucp_gpu_nic_assignment_add_group_not_shared(
ucp_gpu_nic_assignment_t *assignment, const ucs_topo_group_t *group,
ucp_gpu_nic_assignment_policy_t policy)
{
size_t num_gpus = ucs_array_length(&group->gpus);
ucp_gpu_nic_sys_dev_bitmap_t *nic_sys_dev_bitmap;
const ucs_topo_group_element_t *nic;
const ucs_topo_group_element_t *gpu;
size_t nic_idx;
size_t gpu_idx;
const ucs_topo_group_element_t *gpu, *nic;
size_t nic_idx, gpu_idx;

ucs_assertv(policy != UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED,
"invalid policy: %d", (int)policy);

ucs_array_for_each_index(nic, nic_idx, &group->nics) {
gpu_idx = ucp_gpu_nic_assignment_get_gpu_idx(policy, num_gpus, nic_idx);
Expand All @@ -354,6 +406,18 @@ ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment,
}
}

static void
ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment,

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.

remove this and inject policy in the called function, implement shared/not shared in one function

const ucs_topo_group_t *group,
ucp_gpu_nic_assignment_policy_t policy)
{
if (policy == UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED) {
ucp_gpu_nic_assignment_add_group_shared(assignment, group);
} else {
ucp_gpu_nic_assignment_add_group_not_shared(assignment, group, policy);
}
}

ucs_status_t
ucp_gpu_nic_assignment_build(const ucs_topo_groups_t *groups,
ucp_gpu_nic_assignment_policy_t policy,
Expand Down
23 changes: 23 additions & 0 deletions src/ucp/core/ucp_gpu_nic_assignment.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
#ifndef UCP_GPU_NIC_ASSIGNMENT_H_
#define UCP_GPU_NIC_ASSIGNMENT_H_

#include "ucp_types.h"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe define ucp_gpu_nic_assignment_mode_t here next to the closely-coupled policy enum instead of in ucp_types.h, so this self-contained module header does not need to pull in ucp_types.h (and with it ucp/api/ucp.h, uct/api/uct.h)?


#include <ucs/arch/cpu.h>
#include <ucs/datastruct/static_bitmap.h>
#include <ucs/sys/topo/base/topo_groups.h>

Expand Down Expand Up @@ -52,10 +55,30 @@ typedef enum {
*/
UCP_GPU_NIC_ASSIGNMENT_POLICY_ROUND_ROBIN,

/**
* Assign every NIC, with all of its ports, to every GPU in the group.
*/
UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED,

UCP_GPU_NIC_ASSIGNMENT_POLICY_LAST
} ucp_gpu_nic_assignment_policy_t;


/**
* Resolve the GPU-to-NIC assignment policy.
*
* @param [in] mode GPU-to-NIC assignment mode.
* @param [in] cpu_model CPU model used to resolve automatic mode.
* @param [out] policy_p Resolved assignment policy,
* populated only when assignment is enabled.
*
* @return Nonzero if assignment is enabled, or zero if it is disabled.
*/
int ucp_gpu_nic_assignment_policy_resolve(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

int return plus conditionally-written out param means caller and test must track two values for one decision. maybe return the policy directly and use UCP_GPU_NIC_ASSIGNMENT_POLICY_LAST for "disabled", instead of int + out param? the caller becomes one comparison, and the test no longer needs to assert that policy was left untouched.

ucp_gpu_nic_assignment_mode_t mode, ucs_cpu_model_t cpu_model,
ucp_gpu_nic_assignment_policy_t *policy_p);


/**
* Build GPU-to-NIC assignments from topology groups.
*
Expand Down
15 changes: 14 additions & 1 deletion src/ucp/core/ucp_types.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright (c) NVIDIA CORPORATION & AFFILIATES, 2001-2017. ALL RIGHTS RESERVED.
* Copyright (c) NVIDIA CORPORATION & AFFILIATES, 2001-2026. ALL RIGHTS RESERVED.
*
* See file LICENSE for terms.
*/
Expand Down Expand Up @@ -233,6 +233,19 @@ typedef enum {
} ucp_fence_mode_t;


/**
* GPU-to-NIC assignment mode.
*/
typedef enum {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor: these per-value comments repeat the config help string in ucp_context.c and the policy doc in ucp_gpu_nic_assignment.h; docs/CodeStyle.md ("Document once, in the most relevant area") — maybe keep only the config help text and leave a pointer here.

Rationale: the flip/round_robin/shared semantics are now written out in three places, so they will drift when a policy is added or changed.

UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO,
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
UCP_GPU_NIC_ASSIGNMENT_MODE_OFF,
UCP_GPU_NIC_ASSIGNMENT_MODE_FLIP,
UCP_GPU_NIC_ASSIGNMENT_MODE_ROUND_ROBIN,
UCP_GPU_NIC_ASSIGNMENT_MODE_SHARED,
UCP_GPU_NIC_ASSIGNMENT_MODE_LAST
} ucp_gpu_nic_assignment_mode_t;


/**
* Communication scheme in RNDV protocol.
*/
Expand Down
Loading