mtl/ofi: NIC selection update #11338

amirshehataornl · 2023-01-24T02:31:24Z

The original thread of discussion was closed when I deleted my local repo. here it is for reference: #11206

The existing code in compare_cpusets assumed that some non_io ancestor of a PCI object should intersect with the cpuset of the proc. However, this is not true. There is a case where the non IO ancestor can be an L3. If there exists two L3s on the same NUMA and the process is bound to one L3, but the PCI object is connected to the other L3, then compare_cpusets() will return false.

A better way to determine the optimal interface is by finding the distances of the interfaces from the current process. Then find out which of these interfaces is nearest the process and select it.

Use the PMIx distance generation for this purpose.

Signed-off-by: Amir Shehata [email protected]

ompiteam-bot · 2023-01-24T02:31:27Z

Can one of the admins verify this patch?

amirshehataornl · 2023-01-24T02:32:54Z

@rhc54, sorry i deleted my original repo, which closed the original PR. Still wrapping my mind around github workflow. Here is the original PR for reference: #11206

I resynched with latest and greatest to pass the CI (hopefully)

rhc54

Looks like you lost a commit in the process.

rhc54 · 2023-01-24T02:53:29Z

opal/mca/common/ofi/common_ofi.c

+    distances = (pmix_device_distance_t*)dptr->array;
+
+	for (i = 0; i < dptr->size; i++)
+		fprintf(stderr, "%d: %d:%s:%d:%d\n", getpid(), i, distances[i].uuid,


I believe you meant to take this debug out?

rhc54 · 2023-01-24T02:55:47Z

opal/mca/common/ofi/common_ofi.c

+
+	for(osdev = pcidev->io_first_child; osdev != NULL; osdev = osdev->next_sibling) {
+		int i;
+		const char *address = hwloc_obj_get_info_by_name(osdev, "Address");


I think you lost something here - you need to address the two cases: (a) where the device is declared to be "network" (which has the "address" field), and (b) where the device is declared to be "openfabric" (which has "nodeguid" and "systemguid", IIRC).

rhc54 · 2023-01-24T02:58:30Z

FWIW: if you go back to the prior PR (#11206), you can simply recover the diff by adding ".diff" to the PR URL - it sends you to https://patch-diff.githubusercontent.com/raw/open-mpi/ompi/pull/11206.diff. You can then save that diff and apply it to your new repo to fully recover where you were at.

amirshehataornl · 2023-01-24T08:49:17Z

sorry about that. I was working on two different computers and didn't sync with my latest changes. This should bring back the exact same code.

rhc54 · 2023-01-24T14:48:58Z

Looks good to me!

amirshehataornl · 2023-01-24T18:14:37Z

@rhc54, thinking about it some more, is there a situation where Open MPI would not have access to PMIx? IE, it's not compiled with it? Do we need to handle this scenario?

rhc54 · 2023-01-24T18:17:18Z

Beginning with OMPI v5, PMIx support is required. It is possible that the server the app process connects to won't provide distance info - but PMIx support itself must be there.

amirshehataornl · 2023-01-24T18:19:30Z

Should we have a path in the code then, where we check if distance info is available and if not, we calculate it ourselves?

rhc54 · 2023-01-24T18:22:20Z

Safer - or have a fallback that may not be as optimal. Best is both since you may not be able to get the topology or it may not include things you recognize.

rhc54 · 2023-01-25T04:13:49Z

Let me provide a little more direction:

Attempt to get the device distance info directly using PMIx_Get.
If that doesn't succeed, then try to get your cpuset (PMIx_Get_cpuset) and compute the device distances (PMIx_Compute_distances) for yourself.
If that fails (e.g., cannot get a cpuset - you might be unbound - or the distances cannot be computed for some reason), then you need a fallback method for selecting the device to use. In this case, it probably won't matter much which one you use - e.g., if you aren't bound, then they are all equal - so maybe just use a round-robin assignment (e.g., rank % num_devices).

Hope that helps.

amirshehataornl · 2023-01-25T16:30:13Z

@rhc54, thanks. I was thinking along the same lines. I'll update the patch sometime today.

amirshehataornl · 2023-01-26T18:55:19Z

@rhc54, when you have a chance, if you could give it another look to see if it's good to go, that would be great. Thanks.

rhc54 · 2023-01-26T21:24:36Z

Looks okay to me - I think this will do what you wanted 😄

amirshehataornl · 2023-01-27T02:58:14Z

@rhc54 cleaned up one place in the code.
What are the next steps for it to end up landing?

rhc54 · 2023-01-27T03:01:36Z

Need to get someone from the OMPI community to approve CI for it and to review it for commit. Afraid I can't help with either of those as I'm not part of that community. I think @hppritcha or @bwbarrett might fit the bill, or can assign someone they feel appropriate.

hppritcha · 2023-01-27T19:54:38Z

okay to test

amirshehataornl · 2023-01-30T18:35:36Z

@hppritcha, is there anything left for this patch to be done?

hppritcha · 2023-02-21T21:45:06Z

@wckzhang could you review this PR?

wckzhang · 2023-02-21T22:44:23Z

Okay let me look at it today

opal/mca/common/ofi/common_ofi.c

bgoglin · 2023-03-09T14:03:54Z

@amirshehataornl FYI, coming back to the old discussion about Cray NICs in hwloc: I now have access to a machine with Slingshot NICs (nodes identical to Frontier). If you need something from hwloc, I'll be able to look into it. For now I am just going to mark these Ethernet NIC with the "Slingshot" subtype string.

amirshehataornl · 2023-03-11T02:24:51Z

@amirshehataornl FYI, coming back to the old discussion about Cray NICs in hwloc: I now have access to a machine with Slingshot NICs (nodes identical to Frontier). If you need something from hwloc, I'll be able to look into it. For now I am just going to mark these Ethernet NIC with the "Slingshot" subtype string.

Would you be able to review the patch again and see if the changes you're thinking about will break it?

bgoglin · 2023-03-12T22:00:43Z

@amirshehataornl FYI, coming back to the old discussion about Cray NICs in hwloc: I now have access to a machine with Slingshot NICs (nodes identical to Frontier). If you need something from hwloc, I'll be able to look into it. For now I am just going to mark these Ethernet NIC with the "Slingshot" subtype string.

Would you be able to review the patch again and see if the changes you're thinking about will break it?

I don't see anything that would break (you're not looking at the subtype attribute that I will set).

wckzhang · 2023-03-13T20:29:46Z

opal/mca/common/ofi/common_ofi.c

- *     hwloc_cpuset_intersects()
 */
-static bool compare_cpusets(hwloc_topology_t topology, struct fi_pci_attr pci)
+static int calculate_distances(pmix_device_distance_t **distances,


Maybe more appropriately named to discover_devices_and_calc_distances or something

renamed to compute_dev_distances() as we're not really discovering devices. HWLOC has already discovered the topology. We're just using it to calculate the distance to the devices.

wckzhang · 2023-03-13T20:35:06Z

opal/mca/common/ofi/common_ofi.c

-    if (NULL == obj) {
-        goto error;
-    }
+        if (osdev->attr->osdev.type == HWLOC_OBJ_OSDEV_OPENFABRICS) {


I'm not too familiar with this, are these the only two types osdev.type can be?

I believe network devices can only be of these two types.

wckzhang · 2023-03-13T20:42:37Z

opal/mca/common/ofi/common_ofi.c

+                char lsguid[256], lnguid[256];
+                int ret;
+
+                ret = scanf(distances[i].uuid, "fab://%256s::%256s", lnguid, lsguid);


Is this supposed to be sscanf? I can't see why we would want to read from stdin

The existing code in compare_cpusets assumed that some non_io ancestor of a PCI object should intersect with the cpuset of the proc. However, this is not true. There is a case where the non IO ancestor can be an L3. If there exists two L3s on the same NUMA and the process is bound to one L3, but the PCI object is connected to the other L3, then compare_cpusets() will return false. A better way to determine the optimal interface is by finding the distances of the interfaces from the current process. Then find out which of these interfaces is nearest the process and select it. Use the PMIx distance generation for this purpose. Signed-off-by: Amir Shehata <[email protected]>

wckzhang

I'm okay with this, but I'd like to know what testing was done since I'm surprised the scanf didn't get caught on compilation.

amirshehataornl · 2023-03-21T23:01:10Z

I don't have have access to a device which has the type: HWLOC_OBJ_OSDEV_OPENFABRICS. I tested on crusher and the CXI device registers as a NETWORK device. So my tests didn't hit the OPENFABRICS path of the new code.

amirshehataornl · 2023-03-28T21:18:21Z

@hppritcha, @jsquyres Do we need anything else for this PR or is it good to land?

naughtont3 · 2023-03-28T22:05:55Z

@janjust It would be good to have this in v5.0.x branch too once it lands in main. What's proper process for taking to that branch, post another PR for v5.0.x?

wckzhang · 2023-03-28T22:09:14Z

checkout that branch and do a git cherry-pick -x <commit hash> and create a PR against v5.0.x branch

wckzhang · 2023-03-28T22:11:04Z

There are instructions regarding submitting pull requests to branches here - https://github.com/open-mpi/ompi/wiki/SubmittingPullRequests

naughtont3 · 2023-04-05T18:23:25Z

looks like this may cause a problem building main. did a temporary revert for now to avoid breaking main while look into issue.

lrbison · 2023-04-05T19:28:14Z

Here is a log from a failing compile

vidsouza · 2023-04-05T23:04:11Z

Root causes of the issue:

In line 499, a MARCO's definition is used which corresponds to a do while loop. The do statement is not closed with a closing brackets }
In line 616, the struct one of the variables first_child of the struct hwloc_obj_t is accessed incorrectly as io_first_child.

Other than these two main errors, there were few simple warnings that can be looked into but is not related to this PR.

common_ofi.c: In function 'get_nearest_nics':
common_ofi.c:559:19: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     for (i = 0; i < ndist; i++) {
                   ^
common_ofi.c:521:22: warning: unused variable 'topo' [-Wunused-variable]
     pmix_topology_t *topo;
                      ^~~~
common_ofi.c: In function 'opal_common_ofi_select_provider':
common_ofi.c:775:34: warning: unused variable 'cpusets_match' [-Wunused-variable]
     bool provider_found = false, cpusets_match = false;

amirshehataornl · 2023-04-05T23:34:29Z

#11565

1 above: I'll push in a separate PMIx patch to resolve this. I moved away from using these deprecated Macros. using the functions directly
2 above: the intention is to use io_first_child. That field is in the structure (at least in main)

Thanks for pointing out the warnings. Resolved.

github-actions bot added the Target: main label Jan 24, 2023

rhc54 requested changes Jan 24, 2023

View reviewed changes

amirshehataornl force-pushed the distances branch from 2ea2f93 to d22e881 Compare January 24, 2023 08:48

amirshehataornl force-pushed the distances branch from d22e881 to edbc40c Compare January 26, 2023 01:29

amirshehataornl force-pushed the distances branch from edbc40c to cf15573 Compare January 27, 2023 02:57

hppritcha requested a review from wckzhang January 30, 2023 19:22

wckzhang suggested changes Feb 22, 2023

View reviewed changes

opal/mca/common/ofi/common_ofi.c Show resolved Hide resolved

amirshehataornl force-pushed the distances branch from cf15573 to 72b0faa Compare March 11, 2023 02:22

amirshehataornl requested a review from wckzhang March 11, 2023 02:25

wckzhang suggested changes Mar 13, 2023

View reviewed changes

amirshehataornl force-pushed the distances branch from 72b0faa to d919232 Compare March 14, 2023 22:40

amirshehataornl requested a review from wckzhang March 14, 2023 22:40

wckzhang approved these changes Mar 21, 2023

View reviewed changes

naughtont3 merged commit ac2cfc1 into open-mpi:main Apr 4, 2023

This was referenced Apr 5, 2023

v5.0.x-mtl/ofi: NIC selection update #11561

Closed

Revert "mtl/ofi: NIC selection update" #11562

Merged

mtl/ofi: NIC selection update #11338

mtl/ofi: NIC selection update #11338

Uh oh!

Conversation

amirshehataornl commented Jan 24, 2023

Uh oh!

ompiteam-bot commented Jan 24, 2023

Uh oh!

amirshehataornl commented Jan 24, 2023

Uh oh!

rhc54 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhc54 commented Jan 24, 2023

Uh oh!

amirshehataornl commented Jan 24, 2023

Uh oh!

rhc54 commented Jan 24, 2023

Uh oh!

amirshehataornl commented Jan 24, 2023

Uh oh!

rhc54 commented Jan 24, 2023

Uh oh!

amirshehataornl commented Jan 24, 2023

Uh oh!

rhc54 commented Jan 24, 2023

Uh oh!

rhc54 commented Jan 25, 2023

Uh oh!

amirshehataornl commented Jan 25, 2023

Uh oh!

amirshehataornl commented Jan 26, 2023

Uh oh!

rhc54 commented Jan 26, 2023

Uh oh!

amirshehataornl commented Jan 27, 2023

Uh oh!

rhc54 commented Jan 27, 2023

Uh oh!

hppritcha commented Jan 27, 2023

Uh oh!

amirshehataornl commented Jan 30, 2023

Uh oh!

hppritcha commented Feb 21, 2023

Uh oh!

wckzhang commented Feb 21, 2023

Uh oh!

Uh oh!

bgoglin commented Mar 9, 2023

Uh oh!

amirshehataornl commented Mar 11, 2023

Uh oh!

bgoglin commented Mar 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wckzhang left a comment

Choose a reason for hiding this comment

Uh oh!

amirshehataornl commented Mar 21, 2023

Uh oh!

amirshehataornl commented Mar 28, 2023

Uh oh!

naughtont3 commented Mar 28, 2023

Uh oh!

wckzhang commented Mar 28, 2023

vidsouza commented Apr 5, 2023 •

edited

Loading

amirshehataornl commented Apr 5, 2023 •

edited

Loading