ch4: enablement on platforms with 8+ nics #6689

wenduwan · 2023-09-22T02:13:34Z

Pull Request Description

Remove the 8-nic limit and enable MPICH to support high nic-count platforms

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.

wenduwan · 2023-09-22T02:14:02Z

src/mpid/ch4/netmod/ofi/ofi_nic.c

    /* Initially sort the NICs by name. This way all intranode ranks have a consistent view. */
    qsort(MPIDI_OFI_global.prov_use, MPIDI_OFI_global.num_nics, sizeof(struct fi_info *),
          compare_nic_names);

+    if (nic_count > MPIDI_OFI_MAX_NICS) {
+        fprintf(stderr, "Warning: Detected %d NICs, but only %d(MPIDI_OFI_MAX_NICS) are considered.\n",


Please advise a better way to print warnings.

wenduwan · 2023-09-25T17:56:35Z

@hzhou @raffenet Could you kindly review the change?

hzhou · 2023-09-25T18:56:58Z

src/mpid/ch4/netmod/ofi/ofi_pre.h

@@ -284,7 +284,7 @@ typedef struct {
 } MPIDI_OFI_win_t;

 /* Maximum number of network interfaces CH4 can support. */
-#define MPIDI_OFI_MAX_NICS 8
+#define MPIDI_OFI_MAX_NICS 128


I think we can remove this macro and rely on MPIR_CVAR_CH4_OFI_MAX_NICS to control the number of active NICs to use. I think we should reset MPIR_CVAR_CH4_OFI_MAX_NICS default to 8.

@hzhou Cool this is better. I can give it a go by next week.

I would also like second opinions from Aurora folks since they introduced the limit. Do you know a good contact?

cc @zhenggb72 @sagarth

@zhenggb72 @sagarth Some background on this PR. The 8 NIC limit is a blocker for AWS customers to use newer platforms with e.g. 32 NICs. We are exploring the possibility to raise or preferably remove the limit.

Do you see any downside of doing so?

I do like Hui's suggestion. We also went to keep the ability to change the number of NICs with a CVAR, in case we only want to use the NICs on first socket. Its an easier control.

@wenduwan Maybe you could set a machine specific value of this CVAR on your platforms?

@hzhou After reading the usage of MPIDI_OFI_MAX_NICS closer, I think it requires a larger change to eliminate it.

I'm especially curious about these static counters

mpich/src/mpid/ch4/netmod/ofi/globals.c

Lines 10 to 19 in 69522c2

unsigned long long PVAR_COUNTER_nic_sent_bytes_count[MPIDI_OFI_MAX_NICS] ATTRIBUTE((unused));

unsigned long long PVAR_COUNTER_nic_recvd_bytes_count[MPIDI_OFI_MAX_NICS] ATTRIBUTE((unused));

unsigned long long PVAR_COUNTER_striped_nic_sent_bytes_count[MPIDI_OFI_MAX_NICS]

ATTRIBUTE((unused));

unsigned long long PVAR_COUNTER_striped_nic_recvd_bytes_count[MPIDI_OFI_MAX_NICS]

ATTRIBUTE((unused));

unsigned long long PVAR_COUNTER_rma_pref_phy_nic_put_bytes_count[MPIDI_OFI_MAX_NICS]

ATTRIBUTE((unused));

unsigned long long PVAR_COUNTER_rma_pref_phy_nic_get_bytes_count[MPIDI_OFI_MAX_NICS]

ATTRIBUTE((unused));

Do you think we can/should make them dynamically sized instead?

Yes, I think they can be dynamically allocated

@tarudoodi Apologies I missed your comment last week. I have retained the CVAR after the change. Please let me know if that is sufficient for your purpose.

hzhou · 2023-10-04T01:54:10Z

tag @nitbhat @tarudoodi for review

wenduwan · 2023-10-04T20:07:35Z

Converted change to draft for testing.

wenduwan · 2023-10-09T14:21:39Z

src/mpid/ch4/netmod/ofi/ofi_nic.c

+    struct fi_info *first_prov = NULL, *prev_prov = NULL, *cur_prov = NULL;
+    for (cur_prov = prov; cur_prov; cur_prov = cur_prov->next) {
        /* additional filtering */
-        if (MPIR_CVAR_OFI_SKIP_IPV6 && p->addr_format == FI_SOCKADDR_IN6) {
+        if (MPIR_CVAR_OFI_SKIP_IPV6 && cur_prov->addr_format == FI_SOCKADDR_IN6) {
            continue;
        }
-        if (!MPIDI_OFI_nic_is_up(p)) {
+        if (!MPIDI_OFI_nic_is_up(cur_prov)) {
            continue;
        }
-        if (!first_prov) {
-            first_prov = p;
-        }
 #ifdef HAVE_LIBFABRIC_NIC
        /* check the nic */
-        struct fid_nic *nic = p->nic;
+        struct fid_nic *nic = cur_prov->nic;
        if (nic && nic->bus_attr->bus_type == FI_BUS_PCI &&
-            !MPIDI_OFI_nic_already_used(p, MPIDI_OFI_global.prov_use, nic_count)) {
-            MPIDI_OFI_global.prov_use[nic_count] = fi_dupinfo(p);
-            MPIR_Assert(MPIDI_OFI_global.prov_use[nic_count]);
-            nic_count++;
-            if (nic_count == max_nics) {
-                break;
+            !MPIDI_OFI_nic_already_used(cur_prov, first_prov)) {
+            if (prev_prov) {
+                prev_prov->next = fi_dupinfo(cur_prov);
+                prev_prov = prev_prov->next;
+            } else {
+                first_prov = fi_dupinfo(cur_prov);
+                prev_prov = first_prov;
            }
+            prev_prov->next = NULL;
+            ++nic_count;
        }
-#endif
+#else
+        /* Always pick the first provider if NIC info is not available */
+        if (!first_prov) {
+            first_prov = fi_dupinfo(cur_prov);
+            first_prov->next = NULL;
+            ++nic_count;
+            break;
+        }
+#endif /* HAVE_LIBFABRIC_NIC */
+    }


This change can be extracted to a separate commit if you like. I kept it in the big commit since git add -p does not allow me to split it out.

wenduwan · 2023-10-09T14:30:12Z

src/mpid/ch4/netmod/ofi/ofi_pre.h

-#ifdef MPIDI_OFI_VNI_USE_DOMAIN
-    fi_addr_t dest[MPIDI_OFI_MAX_NICS][MPIDI_CH4_MAX_VCIS];     /* [nic][vci] */
-#else
-    fi_addr_t dest[MPIDI_OFI_MAX_NICS][1];
-#endif


In my update I chose to ignore MPIDI_OFI_VNI_USE_DOMAIN - instead I made dest to be always nics x MPIDI_CH4_MAX_VCIS. I believe MPIDI_CH4_MAX_VCIS by default is 1.

wenduwan · 2023-10-09T14:30:35Z

@hzhou Please take a look at the update. I followed your suggestion to deprecate MPIDI_OFI_MAX_NICS. As a result I had to change a few static variables and data structures to make their size and memory dynamic. I tried to make sure the memory is alloced and freed properly.

For my testing, I ran the osu microbenchmarks on 2 different platforms with ./configure ... --enable-g=all

hpc6a.48xlarge with 1 NIC
p5.48xlarge with 32 NICs

I added prints and verified that the code selects the right nic for each rank, and MPIR_CVAR_CH4_OFI_MAX_NICS can be used to restrict NIC visibility.

Please let me know your thoughts.

tarudoodi · 2023-10-10T17:50:55Z

src/mpid/ch4/netmod/ofi/ofi_types.h

@@ -399,7 +399,7 @@ typedef struct {
    /* Process management and PMI globals */
    int pname_set;
    int pname_len;
-    char addrname[MPIDI_OFI_MAX_NICS][FI_NAME_MAX];
+    char addrname[FI_NAME_MAX];


Do we need to account for the new num_nics here too?

@tarudoodi I don't think so. This change is in its own commit. addrname is currently used as a singular char array/string, and I think it was defined as a 2-d array by mistake.

tarudoodi · 2023-10-10T23:45:23Z

@wenduwan The PR looks ok to me. It keeps the usage that we had intended too. Please run the tests to make sure the testsuite passes.

Move MPIDI_av_table decl to mpidpre.h so that it has the same visibility as MPIDI_av_entry Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

MPIDI_OFI_global_t.addrname is used as a single char array, no need to make it a 2-D matrix. Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

This patch removes the MPIDI_OFI_MAX_NICS variable which restricts the max number of NICs that can be used by MPICH. This change requires changing usages of MPIDI_OFI_MAX_NICS and converting statically allocated arrays to dynamic arrays. As a result, malloc and free procredures are added to init and finalize hooks. This patch retains the current behavior of MPIR_CVAR_CH4_OFI_MAX_NICS, which by default uses all NICs available from the OFI provider; the application can restrict the NIC count by setting this CVAR. Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

wenduwan · 2023-10-13T01:12:19Z

@wenduwan The PR looks ok to me. It keeps the usage that we had intended too. Please run the tests to make sure the testsuite passes.

@tarudoodi Thank you for looking! I have applied the whitespace diff and updated the PR.

nitbhat · 2023-10-17T09:55:28Z

tag @nitbhat @tarudoodi for review

Sorry the delay in getting back on this. Reviewed, the changes look good.

Do the CI tests exercise multi-nic code? Otherwise, it'll be good to manually test on some major multi-nic platforms to ensure that nic assignment looks good. (Platforms could be: aws instances, Aurora, Infiniband systems with multiple nics).

wenduwan commented Sep 22, 2023

View reviewed changes

wenduwan force-pushed the max_ofi_nics branch from 919ff2c to 942f58e Compare September 25, 2023 17:08

hzhou reviewed Sep 25, 2023

View reviewed changes

hzhou requested review from nitbhat and tarudoodi October 4, 2023 01:54

wenduwan marked this pull request as draft October 4, 2023 20:07

wenduwan force-pushed the max_ofi_nics branch from 942f58e to b4fbfcf Compare October 9, 2023 14:15

wenduwan commented Oct 9, 2023

View reviewed changes

wenduwan marked this pull request as ready for review October 9, 2023 14:22

wenduwan commented Oct 9, 2023

View reviewed changes

tarudoodi reviewed Oct 10, 2023

View reviewed changes

wenduwan added 3 commits October 13, 2023 01:02

ch4: lift MPIDI_av_table decl to mpidpre.h

34c74aa

Move MPIDI_av_table decl to mpidpre.h so that it has the same visibility as MPIDI_av_entry Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

ch4/ofi: change MPIDI_OFI_global_t.addrname type

765ae7f

MPIDI_OFI_global_t.addrname is used as a single char array, no need to make it a 2-D matrix. Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

wenduwan force-pushed the max_ofi_nics branch from b4fbfcf to 22ba834 Compare October 13, 2023 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ch4: enablement on platforms with 8+ nics #6689

ch4: enablement on platforms with 8+ nics #6689

wenduwan commented Sep 22, 2023 •

edited

wenduwan Sep 22, 2023

wenduwan commented Sep 25, 2023

hzhou Sep 25, 2023

wenduwan Sep 26, 2023

hzhou Sep 26, 2023

wenduwan Sep 26, 2023

tarudoodi Oct 4, 2023

wenduwan Oct 4, 2023

hzhou Oct 4, 2023

wenduwan Oct 9, 2023

hzhou commented Oct 4, 2023

wenduwan commented Oct 4, 2023

wenduwan Oct 9, 2023

wenduwan Oct 9, 2023

wenduwan commented Oct 9, 2023

tarudoodi Oct 10, 2023

wenduwan Oct 10, 2023

tarudoodi commented Oct 10, 2023

wenduwan commented Oct 13, 2023

nitbhat commented Oct 17, 2023

	unsigned long long PVAR_COUNTER_nic_sent_bytes_count[MPIDI_OFI_MAX_NICS] ATTRIBUTE((unused));
	unsigned long long PVAR_COUNTER_nic_recvd_bytes_count[MPIDI_OFI_MAX_NICS] ATTRIBUTE((unused));
	unsigned long long PVAR_COUNTER_striped_nic_sent_bytes_count[MPIDI_OFI_MAX_NICS]
	ATTRIBUTE((unused));
	unsigned long long PVAR_COUNTER_striped_nic_recvd_bytes_count[MPIDI_OFI_MAX_NICS]
	ATTRIBUTE((unused));
	unsigned long long PVAR_COUNTER_rma_pref_phy_nic_put_bytes_count[MPIDI_OFI_MAX_NICS]
	ATTRIBUTE((unused));
	unsigned long long PVAR_COUNTER_rma_pref_phy_nic_get_bytes_count[MPIDI_OFI_MAX_NICS]
	ATTRIBUTE((unused));

ch4: enablement on platforms with 8+ nics #6689

Are you sure you want to change the base?

ch4: enablement on platforms with 8+ nics #6689

Conversation

wenduwan commented Sep 22, 2023 • edited

Pull Request Description

Author Checklist

Choose a reason for hiding this comment

wenduwan commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzhou commented Oct 4, 2023

wenduwan commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenduwan commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarudoodi commented Oct 10, 2023

wenduwan commented Oct 13, 2023

nitbhat commented Oct 17, 2023

wenduwan commented Sep 22, 2023 •

edited