ch4/ofi: fix distribution of NICs in NUMA systems - #7908
Conversation
375ed8d to
d005ea7
Compare
hzhou
left a comment
There was a problem hiding this comment.
Looks good in general! Curious on how it works in your testing with 32 NICs.
| /* 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can make this more robust
There was a problem hiding this comment.
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.
b28e245 to
55d4f76
Compare
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. |
a35248a to
bed6dd6
Compare
| * - 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 |
There was a problem hiding this comment.
An alternative solution is to rely on asynchronous shared memory table using inter-process mutex. The shared memory slab is created here:
mpich/src/mpid/ch4/shm/posix/posix_init.c
Lines 287 to 300 in c2e6701
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:
- 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
- a selection algorithm that avoids already selected nic and the nics that would be selected by the ranks that has the precedence.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
e43582a to
733db67
Compare
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.
733db67 to
57d5fcd
Compare
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
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits are self-contained and do not do two things at once.
Commit message is of the form:
module: short descriptionCommit message explains what's in the commit.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.