ch4: default MPIR_CVAR_CH4_ROOTS_ONLY_PMI on #7110
Conversation
|
test:mpich/ch4/all |
|
test:mpich/ch4/ofi |
| if (MPIDI_OFI_ENABLE_AV_TABLE) { | ||
| for (int r = 0; r < size; r++) { | ||
| MPIDI_OFI_addr_t *av ATTRIBUTE((unused)) = &MPIDI_OFI_AV(&MPIDIU_get_av(0, r)); | ||
| MPIR_Assert(av->dest[0][0] == get_root_av_table_index(r)); |
There was a problem hiding this comment.
what exactly is this verifying? I'm confused why the index into av->dest doesn't change with the rank r.
There was a problem hiding this comment.
The av is the entry associated with the rank r.
There was a problem hiding this comment.
The reason we do this checking is explained in the comments at the front of the file. We compressed the dimensions of the av table based on the assumption that all the remote av are inserted exactly with the same order on each local context (vci, nic), thus they should have exactly the same fi_addr. The check confirms this assumption is still correct.
There was a problem hiding this comment.
OK. av->dest[0][0] has type fi_addr_t if I'm not mistaken? And we are comparing that with an integer rank?
There was a problem hiding this comment.
Yes, it is an integer under MPIDI_OFI_ENABLE_AV_TABLE. We are not comparing it to rank, it is the index to the av table.
Use ROOTS_ONLY_PMI significantly improves the large scale init time. It is about time to make it default.
Since we moved multi vci init to MPIDI_world_post_init, we should dump the debug info for multi vcis to MPIDI_OFI_post_init as well.
The logic in calculating index for the MPIR_CVAR_CH4_ROOTS_ONLY_PMI was incorrect. It leaked through CI testing because the check was only done in MPIDI_OFI_addr_exchange_all_ctx, which only runs with multiple vci, and we never tested the combination. * split get_av_table_index and get_root_av_table_index and add assertion check in MPIDI_OFI_addr_exchange_root_ctx. * fix the av index calculation not assuming processes ranks are clustered in each node.
* In MPIDU_bc_allgather when we overwrite bc_len due to not of same_len, we need fillout the bc_table according to the new max bc_len. * The bc_table after MPIDU_bc_allgather is not in the rank-order, but in the node-consecutive order. We need use rank_map to look up the table slot.
|
Thanks for the review @raffenet . The tests are not clean, but all of them are testing congestion issue that are not related to this PR. I'll merge now. If there are recurring testing issues, we'll address them afterward. |
Pull Request Description
Using ROOTS_ONLY_PMI significantly improves the large-scale init time. It is about time to make it default.
[skip warnings]
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.