Repository navigation
UCP/CORE: Add shared topology assignment policy - #11980
Conversation
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| * GPU network device assignment mode. | ||
| */ | ||
| typedef enum { | ||
| UCP_GPU_NET_DEVICE_MODE_NOT_SHARED, |
There was a problem hiding this comment.
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.
| config.num_gpus_per_group = num_gpus_per_group; | ||
| config.num_nics_per_group = num_nics_per_group; | ||
|
|
||
| for (size_t num_gpu_devices = 1; num_gpu_devices <= 2; |
There was a problem hiding this comment.
minor: same shape loop as check_assignment_not_shared; can we keep one loop helper and pass the per-shape check, so the bound comments are not lost?
|
🤖 CI Triage Agent — TL;DR: The Coverity gate failed with 2 new Full analysisSummary: Azure Pipelines job "coverity devel on coverity_rh7" (build 137012) failed at the end of Root cause: The build itself succeeded ("745 C/C++ compilation units (100%) successfully",
In practice both divisions are unreachable (with Implicated commit: File: Suggested fix: Remove the divisions so Coverity has nothing to flag — iterate groups explicitly instead of deriving the group index by division, e.g.: for (size_t group_idx = 0; group_idx < config.num_groups; ++group_idx) {
for (size_t local_gpu = 0; local_gpu < config.num_gpus_per_group; ++local_gpu) {
const size_t gpu_idx = (group_idx * config.num_gpus_per_group) + local_gpu;
...
for (size_t nic_group_idx = 0; nic_group_idx < config.num_groups; ++nic_group_idx) {
for (size_t local_nic = 0; local_nic < config.num_nics_per_group; ++local_nic) {
const size_t nic_idx = (nic_group_idx * config.num_nics_per_group) + local_nic;
...
}
}
}
}If you prefer a minimal patch, make the non-zero invariants explicit right where the degenerate cases are already handled, so the checker sees them: if (config.num_gpus() == 0) { ...existing...; return; }
ASSERT_NE(0ul, config.num_gpus_per_group); /* implied by num_gpus() != 0 */
...
if (config.num_nics() == 0) {
EXPECT_EQ(0ul, static_cast<size_t>(UCS_STATIC_BITMAP_POPCOUNT(*nic_sys_dev_bitmap)));
continue;
}
ASSERT_NE(0ul, config.num_nics_per_group); /* before line 313 */Avoid suppressing via a Coverity annotation unless the restructure is impractical. After the change, re-run the Coverity stage; the published Related: PR #11980 (this change); prior GPU/NIC assignment PR #11868. |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| const std::vector<size_t> &expected_owners) | ||
| void check_assignment_shared_inner(const topology_shape_t &config) | ||
| { | ||
| ASSERT_NE(config.num_groups, 0); |
There was a problem hiding this comment.
these preconditions are copied from check_assignment_not_shared_inner(); maybe move them into the check_assignment() template so both paths share one copy.
|
Residual coverage note: the new |
| " - not_shared : use the default FLIP assignment.\n" | ||
| " - not_shared_flip : use the FLIP assignment.\n" |
There was a problem hiding this comment.
What's the difference? Why are there two options for this?
There was a problem hiding this comment.
This is the original design, I'm looking into changing this to something more intuitive
|
|
||
|
|
||
| ucp_gpu_nic_assignment_policy_t | ||
| ucp_gpu_net_device_mode_get_policy(ucp_gpu_net_device_mode_t mode) |
There was a problem hiding this comment.
Is there really a need for both ucp_gpu_net_device_mode_t and ucp_gpu_nic_assignment_policy_t?
There was a problem hiding this comment.
It is required now after the addition of auto and off
|
🤖 CI Triage Agent — TL;DR: The Coverity gate failed with 2 new DIVIDE_BY_ZERO defects in Full analysisSummary: Azure job "coverity devel on coverity_rh7" (build 137036) failed at the end of Root cause: The two new tests added by this PR feed zero-sized topology shapes into the shared-policy checker:
At runtime both divisions are actually unreachable ( Implicated commit: [REDACTED:Hex High Entropy String] "TEST/UCP: Add check_assignment template" (Guy Ealey Morag) exposed the path; the divisions and the zero-argument tests come from 3f1fa07 "UCP/CORE: Add shared topology assignment policy" (Guy Ealey Morag). File: Suggested fix: Make the zero cases explicit in check_gpu_device_aliases(config);
/* No NICs: every GPU must map to an empty bitmap, no division needed */
if (config.num_nics() == 0) {
for (size_t gpu_idx = 0; gpu_idx < config.num_gpus(); ++gpu_idx) {
const auto *bitmap = ucp_gpu_nic_assignment_lookup(
&m_assignment, gpu_sys_dev(config, gpu_idx, 0));
ASSERT_NE(nullptr, bitmap);
EXPECT_EQ(0, UCS_STATIC_BITMAP_POPCOUNT(*bitmap));
}
return;
}
/* Reached only when both per-group counts are non-zero */
ASSERT_NE(0ul, config.num_gpus_per_group);
ASSERT_NE(0ul, config.num_nics_per_group);placed right after the existing Related: PR #11980 (branch
|
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| switch (mode) { | ||
| case UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO: | ||
| if (cpu_model != UCS_CPU_MODEL_NVIDIA_VERA) { |
There was a problem hiding this comment.
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?
| "Select how NICs are assigned to GPUs.\n" | ||
| " - auto : enable automatically on supported hardware, otherwise disable.\n" | ||
| " - off : disable GPU-to-NIC assignment, fallback to free lane selection.\n" | ||
| " - flip : require the not shared flip assignment policy.\n" |
There was a problem hiding this comment.
minor: "require the not shared flip assignment policy" is hard to parse; pls describe what each policy does.
| " - 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.", |
| const std::vector<size_t> &expected_owners) | ||
| void check_assignment_shared_inner(const topology_shape_t &config) | ||
| { | ||
| ASSERT_NE(config.num_groups, 0); |
There was a problem hiding this comment.
minor: these asserts are copied from check_assignment_not_shared_inner(); can we move them into the common check_assignment() wrapper instead?
|
Residual coverage note: The new |
|
|
||
| {"GPU_NIC_ASSIGNMENT_MODE", "auto", | ||
| "Select how NICs are assigned to GPUs.\n" | ||
| " - auto : enable automatically on supported hardware, otherwise disable.\n" |
There was a problem hiding this comment.
| " - 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?
There was a problem hiding this comment.
In general the policy can be adapted automatically according to whatever hw is detected, so I decided to not be too specific here.
|
|
||
| switch (mode) { | ||
| case UCP_GPU_NIC_ASSIGNMENT_MODE_AUTO: | ||
| if (cpu_model != UCS_CPU_MODEL_NVIDIA_VERA) { |
There was a problem hiding this comment.
This happens implicitly with the other policies too, no? Is this really needed if the topology groups aren't being created on vera anyway?
There was a problem hiding this comment.
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
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Coverity static analysis ( Root cause: The build/compile and link succeeded ("745 C/C++ compilation units (100%) successfully", "cov-build utility completed successfully"); the failure is purely the analysis gate. Coverity traces:
At runtime these paths are unreachable: Implicated commit: File: Suggested fix: Make the zero-divisor guards visible to Coverity by turning the gtest assertions into real control flow, e.g.: // was: ASSERT_NE(config.num_gpus_per_group, 0);
if (config.num_gpus_per_group == 0) {
ADD_FAILURE() << "num_gpus_per_group must be non-zero";
return;
}
...
// was: ASSERT_NE(config.num_nics_per_group, 0);
if (config.num_nics_per_group == 0) {
ADD_FAILURE() << "num_nics_per_group must be non-zero";
return;
}Do this before the loop at line 296 for Related: PR #11980 (
|
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| {"GPU_NIC_ASSIGNMENT_MODE", "auto", | ||
| "Select how NICs are assigned to GPUs.\n" | ||
| " - auto : enable automatically on supported hardware, otherwise disable.\n" |
There was a problem hiding this comment.
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:
| " - 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.", |
|
|
||
| check_gpu_device_aliases(config); | ||
|
|
||
| if (config.num_gpus_per_group == 0) { |
There was a problem hiding this comment.
minor: pls remove this and the num_nics_per_group == 0 check below; num_groups is asserted non-zero and num_gpus() == num_groups * num_gpus_per_group, so the num_gpus() == 0 / num_nics() == 0 early returns above already cover those cases and these ADD_FAILURE() branches are dead code (contradicts the compactness rule in REVIEW.md).
| #ifndef UCP_GPU_NIC_ASSIGNMENT_H_ | ||
| #define UCP_GPU_NIC_ASSIGNMENT_H_ | ||
|
|
||
| #include "ucp_types.h" |
There was a problem hiding this comment.
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)?
| 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", |
There was a problem hiding this comment.
for my understanding: can you pls remind me when/why the flip policy is needed?
There was a problem hiding this comment.
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_nics_string_buffers_t; | ||
|
|
||
|
|
||
| int ucp_gpu_nic_assignment_policy_resolve( |
There was a problem hiding this comment.
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);| 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, |
There was a problem hiding this comment.
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);
}
}
}| } | ||
|
|
||
| static void | ||
| ucp_gpu_nic_assignment_add_group(ucp_gpu_nic_assignment_t *assignment, |
There was a problem hiding this comment.
remove this and inject policy in the called function, implement shared/not shared in one function
| UCP_GPU_NIC_ASSIGNMENT_POLICY_LAST; | ||
|
|
||
| EXPECT_EQ(params.expected_enabled, | ||
| ucp_gpu_nic_assignment_policy_resolve(params.mode, |
There was a problem hiding this comment.
can we remove that test that checks one enum mapping function, and rely on more behavioral test like below?
| } | ||
|
|
||
| UCS_TEST_F(test_ucp_gpu_nic_assignment, clique_flip_divisible) { | ||
| UCS_TEST_F(test_ucp_gpu_nic_assignment, not_shared_no_gpus) { |
There was a problem hiding this comment.
pls double check but with a single add_group() as suggested these can go away from the test:
- shared_no_nics and shared_no_gpus: a zero nic/gpu group now runs the same loop the not_shared tests already cover
- check_assignment_shared_inner(): it duplicates the preflight asserts, build, empty gpu case and device aliases check
- the templated check_assignment() wrapper and the not_shared/shared pair, so the existing check_clique_assignment() calls stay as they are
only the shared test itself is needed, for "every nic goes to every gpu of the group".
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| ucs_assert((gpu->num_sys_devs > 0) && | ||
| (gpu->sys_devs[0] != UCS_SYS_DEVICE_ID_UNKNOWN)); | ||
|
|
||
| nic_sys_dev_bitmap = ucs_const_cast( |
There was a problem hiding this comment.
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".
| * | ||
| * @return Nonzero if assignment is enabled, or zero if it is disabled. | ||
| */ | ||
| int ucp_gpu_nic_assignment_policy_resolve( |
There was a problem hiding this comment.
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.
|
|
||
| check_gpu_device_aliases(config); | ||
|
|
||
| if (config.num_gpus_per_group == 0) { |
There was a problem hiding this comment.
dead code in the new shared checker: both this guard and the num_nics_per_group == 0 one below are unreachable: num_gpus_per_group == 0 implies num_gpus() == 0, which already returned above (same for nics). pls drop them.
| "Select how NICs are assigned to GPUs.\n" | ||
| " - auto : enable automatically on supported hardware, otherwise disable.\n" | ||
| " - off : disable GPU-to-NIC assignment, fallback to free lane selection.\n" | ||
| " - flip : require the not shared flip assignment policy.\n" |
There was a problem hiding this comment.
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.",
|
Residual coverage note: the |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| } ucp_nics_string_buffers_t; | ||
|
|
||
|
|
||
| int ucp_gpu_nic_assignment_policy_resolve( |
There was a problem hiding this comment.
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.
| static UCS_CONFIG_DEFINE_ARRAY(memunit_sizes, sizeof(size_t), | ||
| UCS_CONFIG_TYPE_MEMUNITS); | ||
|
|
||
| /* clang-format off */ |
There was a problem hiding this comment.
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.
| /** | ||
| * GPU-to-NIC assignment mode. | ||
| */ | ||
| typedef enum { |
There was a problem hiding this comment.
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.
|
Considered and dropped: Considered and dropped: the unused-config-knob concern (nothing consumes Considered and dropped (preference-level only, no concrete impact): the Verified correct (no action needed): the new Residual coverage note: |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
minor: can we assert on the range instead, so adding a mode does not require updating this list too? Nothing consumes
Residual coverage note: the |
| UCP_GPU_NIC_ASSIGNMENT_MODE_OFF, | ||
|
|
||
| /** | ||
| * Assign each NIC, with all of its ports, going forward and then backward |
There was a problem hiding this comment.
would be good to mention that flip leverage the fact that some hca have immediate access to SSD memory (or better framing)
There was a problem hiding this comment.
Deferred to another PR
| "for a GPU's memory to the NICs assigned to that GPU.\n" | ||
| "All ports of a NIC are assigned together.\n" | ||
| "The 'flip', 'round_robin' and 'shared' modes apply on any hardware.\n" | ||
| "With 'flip' and 'round_robin', a group with fewer NICs than GPUs leaves\n" |
There was a problem hiding this comment.
mention somewhere that flip intends to leverage the fact that some hca have immediate access to an SSD hard driver under same pcie switch (or smaller/better framing)
There was a problem hiding this comment.
Deferred to another PR
| {"GPU_NIC_ASSIGNMENT_MODE", "auto", | ||
| "Assign NICs to GPUs within each topology group, and restrict the lanes\n" | ||
| "for a GPU's memory to the NICs assigned to that GPU.\n" | ||
| "All ports of a NIC are assigned together.\n" |
There was a problem hiding this comment.
also we can mention that All GPUs with same BDF (MLOParts etc.) share same NIC mapping, or equivalent
There was a problem hiding this comment.
Deferred to another PR
| "Assign NICs to GPUs within each topology group, and restrict the lanes\n" | ||
| "for a GPU's memory to the NICs assigned to that GPU.\n" | ||
| "All ports of a NIC are assigned together.\n" | ||
| "The 'flip', 'round_robin' and 'shared' modes apply on any hardware.\n" |
There was a problem hiding this comment.
we now add forced modes on non-vera hardware ending up with no nics at all due to cx9 filter. we need to add like below and we can later always add other filters if needed.
--- a/src/ucs/sys/topo/base/topo_groups.c
+++ b/src/ucs/sys/topo/base/topo_groups.c
@@
#include <ucs/algorithm/qsort_r.h>
+#include <ucs/arch/cpu.h>
#include <ucs/datastruct/array.h>
@@ ucs_topo_build_groups_inner(const ucs_topo_sys_device_info_t *devices,
{
ucs_topo_group_t inventory;
ucs_topo_groups_t groups;
ucs_status_t status;
+ int is_vera_rubin;
ucs_topo_init_groups(&groups);
- status = ucs_topo_groups_inventory_build(devices, num_devices, 1,
- &inventory);
+ /* The cx9 filter only applies to hardware with a known GPU-NIC topology. */
+ is_vera_rubin = (ucs_arch_get_cpu_model() == UCS_CPU_MODEL_NVIDIA_VERA);
+
+ status = ucs_topo_groups_inventory_build(devices, num_devices,
+ is_vera_rubin, &inventory);There was a problem hiding this comment.
Deferred to another PR
tvegas1
left a comment
tvegas1
left a comment
There was a problem hiding this comment.
need to address last four comments in separate PR
What?
UCX_GPU_NIC_ASSIGNMENT_MODEto control how NICs are assigned to GPUs:auto(default),off,flip,round_robinandshared.sharedmode, which assigns all NICs of a topology group to every GPU in that group.autousesflipon Vera and disables the assignment on other CPUs.autoandoffare handled once, inucp_context_gpu_nic_assignment_init().ucp_gpu_nic_assignment_add_group().sharedmode.