Skip to content

ch4/ofi: fix distribution of NICs in NUMA systems - #7908

Open
mthyoung-amzn wants to merge 1 commit into
pmodels:mainfrom
mthyoung-amzn:fix-close-nic-assignment
Open

ch4/ofi: fix distribution of NICs in NUMA systems#7908
mthyoung-amzn wants to merge 1 commit into
pmodels:mainfrom
mthyoung-amzn:fix-close-nic-assignment

Conversation

@mthyoung-amzn

@mthyoung-amzn mthyoung-amzn commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

On systems with non-uniform attachment of NICs to CPU cores, we are considering closeness in NIC assignment, but assigning them by round-robin based on local rank. In a two-NUMA node system (using -map-by numa) this results in only half of the NICs being used (the even-indexed ones on NUMA node 0 and odd-indexed on NUMA node 1).

This fixes this by explicitly sharing the close NICs across all local ranks, and selecting NICs based on index within each "sharing set" of local ranks.

Pull Request Description

Author Checklist

  • Provide Description
    Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
  • Commits Follow Good Practice
    Commits are self-contained and do not do two things at once.
    Commit message is of the form: module: short description
    Commit message explains what's in the commit.
  • Passes All Tests
    Whitespace checker. Warnings test. Additional tests via comments.
  • Contribution Agreement
    For non-Argonne authors, check contribution agreement.
    If necessary, request an explicit comment from your companies PR approval manager.

@mthyoung-amzn
mthyoung-amzn force-pushed the fix-close-nic-assignment branch 2 times, most recently from 375ed8d to d005ea7 Compare July 29, 2026 16:53

@hzhou hzhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good in general! Curious on how it works in your testing with 32 NICs.

Comment thread src/mpid/ch4/netmod/ofi/ofi_nic.c Outdated
Comment thread src/mpid/ch4/netmod/ofi/ofi_init.c Outdated
/* TODO: run collective and decide all_need_init (as well as num_nics, close_nic_map) */
node_comm = MPIR_Comm_get_node_comm(comm);
if (node_comm && node_comm->local_size > 1) {
all_need_init = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With MPI sessions, users can create a comm_world after a sub-comm already created. So we will have a situation that fabric_initialized are not uniform. Since we can't reorder nics after some of the process already initialized, every processes (in the node_comm) needs to skip multi-nic ordering.

Thus:

if (fabric_initialized) {
    all_need_init = false;
}

My original design was to "allreduce" !fabric_initialized -> all_need_init.

PS: on second thought, I think it is perfectly okay for any not-yet-initialized process to locally re-order nics to select its global closest rank. It may not be perfect since it may collide with those process that already initialized, but that won't be any worse than without re-ordering. Thus, it may be okay to skip all_need_init check altogether.

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 can make this more robust

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 agree that this is correct/safe as-is, though far from optimal. I have rolled back the all_need_init, since in the current implementation it's just kind of confusing. Added a comment to the effect that the Session model needs some work.

I think the right way to do it would be to maintain a shared-memory claim table for all local NICs, so that subsequent communicator creation can see which NICs are already in use by other communicators. That's probably too much for this PR, though.

@mthyoung-amzn
mthyoung-amzn force-pushed the fix-close-nic-assignment branch 3 times, most recently from b28e245 to 55d4f76 Compare July 29, 2026 23:00
@mthyoung-amzn

Copy link
Copy Markdown
Contributor Author

Curious on how it works in your testing with 32 NICs.

This works well! One thing that I want to do (this was in my earlier PR) is to distribute NIC assignments based on more detailed bus topology; we can't actually push full line rate out of all NICs at the same time, but can do better if we distribute NICs in use across host bridges. Getting this right is tricky, so figured it should go in its own PR.

Also, getting GPU/NIC affinity right is quite a bit more important on these systems, so I will be focusing on that next.

Comment thread src/mpid/ch4/netmod/ofi/ofi_init.c Outdated
@mthyoung-amzn
mthyoung-amzn force-pushed the fix-close-nic-assignment branch 2 times, most recently from a35248a to bed6dd6 Compare July 30, 2026 18:01
* - shared memory/atomics-based NIC claim table. Optimal NIC assignment,
* non-deterministic.
* - hybrid of the two; claim table used across node_comm initialization to avoid
* suboptimal NIC assignment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An alternative solution is to rely on asynchronous shared memory table using inter-process mutex. The shared memory slab is created here:

slab = MPL_initshm_open(MPIDI_POSIX_global.shm_name, slab_size, &is_root);
MPIR_ERR_CHKANDJUMP(!slab, mpi_errno, MPI_ERR_OTHER, "**nomem");
if (is_root) {
memset(slab, 0, sizeof(MPIDI_POSIX_shm_t));
MPL_atomic_relaxed_store_uint64(&slab->shm_limit_counter, 0);
MPL_atomic_store_int(&slab->num_shared, 1);
MPL_atomic_store_int(&slab->num_shared_vci, 0);
MPL_atomic_store_int(&slab->shm_ready, MPIDI_POSIX_READY_FLAG);
} else {
while (MPL_atomic_load_int(&slab->shm_ready) != MPIDI_POSIX_READY_FLAG) {
MPID_Thread_yield();
}
MPL_atomic_fetch_add_int(&slab->num_shared, 1);

We can add a mutex and nic usage table to the slab, then each process can select its nic based on previous nic claims by other processes.

My original suggestion is to let every process proceed to MPIDI_OFI_order_multi_nic_global and perform the allgather. The allgather can collect close-nic-bitmaps plus the fabric_initialized flag, then each process can make its nic choice locally.

If we want the algorithm to work for sessions, we can expand the "bitmap" into double purpose:

  1. designate a single bit as special flag
    • if true: the process already selected a preferred nic, and the bitmap marks the selected nic
    • if false: the process have not selected a preferred nic yet, the bitmap marks all the close nics
  2. a selection algorithm that avoids already selected nic and the nics that would be selected by the ranks that has the precedence.

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 we are on the same page regarding the shared memory approach; I would like to do something like that as future work.

As for the current PR, I see what you mean; I'll make the fabric_initialized change.

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.

Okay I removed the added allreduce of fabric_initialized, added a dryrun argument for MPIDI_OFI_order_multi_nic_global and friends. If a process has already initialized the fabric, it will participate in the close-NIC allgather, but skip any reordering of it's NICs

Comment thread src/mpid/ch4/netmod/ofi/ofi_nic.c Outdated
@mthyoung-amzn
mthyoung-amzn force-pushed the fix-close-nic-assignment branch 2 times, most recently from e43582a to 733db67 Compare July 30, 2026 21:46
On systems with non-uniform attachment of NICs to CPU cores, we are
considering closeness in NIC assignment, but assigning them by
round-robin based on local rank. In a two-NUMA node system (using -map-by
numa) this results in only half of the NICs being used (the even-indexed
ones on NUMA node 0 and odd-indexed on NUMA node 1). Other mappings
exhibit different odd behavior.

This improves by explicitly sharing the close NICs across all local
ranks, and selecting NICs based on index within each "sharing set" of
local ranks.
@mthyoung-amzn
mthyoung-amzn force-pushed the fix-close-nic-assignment branch from 733db67 to 57d5fcd Compare July 30, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants