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

static const char *ucp_gpu_net_device_modes[] = {
[UCP_GPU_NET_DEVICE_MODE_NOT_SHARED] = "not_shared",
[UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_FLIP] = "not_shared_flip",
[UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_ROUND_ROBIN] = "not_shared_round_robin",
[UCP_GPU_NET_DEVICE_MODE_SHARED] = "shared",
[UCP_GPU_NET_DEVICE_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 @@ -615,6 +623,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_NET_DEVICE_MODE", "not_shared",
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved
"Select how network devices are assigned to GPUs.\n"
" - not_shared : use the default FLIP assignment.\n"
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved
" - not_shared_flip : use the FLIP assignment.\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.

What's the difference? Why are there two options for this?

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.

This is the original design, I'm looking into changing this to something more intuitive

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.

@tomerg-nvidia Updated

" - not_shared_round_robin : use the round-robin assignment.\n"
" - shared : assign every network device in a topology\n"
" group to every GPU in that group.",
ucs_offsetof(ucp_context_config_t, gpu_net_device_mode),
UCS_CONFIG_TYPE_ENUM(ucp_gpu_net_device_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
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 network device assignment mode */
ucp_gpu_net_device_mode_t gpu_net_device_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
65 changes: 58 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,22 @@ typedef struct {
} ucp_nics_string_buffers_t;


ucp_gpu_nic_assignment_policy_t
ucp_gpu_net_device_mode_get_policy(ucp_gpu_net_device_mode_t mode)

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.

Is there really a need for both ucp_gpu_net_device_mode_t and ucp_gpu_nic_assignment_policy_t?

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 is required now after the addition of auto and off

{
switch (mode) {
case UCP_GPU_NET_DEVICE_MODE_NOT_SHARED:
case UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_FLIP:
return UCP_GPU_NIC_ASSIGNMENT_POLICY_FLIP;
case UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_ROUND_ROBIN:
return UCP_GPU_NIC_ASSIGNMENT_POLICY_ROUND_ROBIN;
case UCP_GPU_NET_DEVICE_MODE_SHARED:
return UCP_GPU_NIC_ASSIGNMENT_POLICY_SHARED;
default:
ucs_fatal("invalid gpu network device 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 +343,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 +393,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
18 changes: 18 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,8 @@
#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/datastruct/static_bitmap.h>
#include <ucs/sys/topo/base/topo_groups.h>

Expand Down Expand Up @@ -52,10 +54,26 @@ 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;


/**
* Get the assignment policy of a GPU network device mode.
*
* @param [in] mode GPU network device mode.
*
* @return Assignment policy corresponding to @a mode.
*/
ucp_gpu_nic_assignment_policy_t
ucp_gpu_net_device_mode_get_policy(ucp_gpu_net_device_mode_t mode);


/**
* Build GPU-to-NIC assignments from topology groups.
*
Expand Down
14 changes: 13 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,18 @@ typedef enum {
} ucp_fence_mode_t;


/**
* GPU network device 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_NET_DEVICE_MODE_NOT_SHARED,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not_shared and not_shared_flip map to the same policy — IMO we can keep only not_shared and use ucp_gpu_nic_assignment_policy_t directly in the config table, dropping this enum and ucp_gpu_net_device_mode_get_policy(). If both are kept, pls document the difference here, like the other mode enums in this file.

UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_FLIP,
Comment thread
guy-ealey-morag marked this conversation as resolved.
Resolved
UCP_GPU_NET_DEVICE_MODE_NOT_SHARED_ROUND_ROBIN,
UCP_GPU_NET_DEVICE_MODE_SHARED,
UCP_GPU_NET_DEVICE_MODE_LAST
} ucp_gpu_net_device_mode_t;


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