V5.0.x: ofi: NIC selection update (revised) #11739

amirshehataornl · 2023-06-06T16:44:28Z

Port of the distances patches to OMPI 5.0.x release

github-actions · 2023-06-06T16:45:04Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

52aa19b: Do not compute distances when unbound

check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

7884ac2: ofi: NIC selection update

check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2023-06-06T16:49:51Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

6626503: Do not compute distances when unbound

check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

eb2c108: ofi: Fix Coverity warnings

check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

7884ac2: ofi: NIC selection update

check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

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. Move away from using deprecated PMIX macros and use the functions directly instead. Signed-off-by: Amir Shehata <shehataa@ornl.gov> (cherry picked from commit d4e1ae5)

Fix two coverity warnings: - Uninitialized variables (UNINIT) in /opal/mca/common/ofi/common_ofi.c: 859 in opal_common_ofi_select_provider() - Null pointer dereferences (FORWARD_NULL) /opal/mca/common/ofi/common_ofi.c: 636 in is_near() Move the "near" parameter in opal_common_ofi_select_provider() outside the #if OPAL_OFI_PCI_DATA_AVAILABLE block, because later in the code it's being used outside of it. Signed-off-by: Amir Shehata <shehataa@ornl.gov> (cherry picked from commit 47f18dc)

Unbound procs cannot have a device distance Signed-off-by: Ralph Castain <rhc@pmix.org> (cherry picked from commit 8a446f6)

naughtont3 · 2023-06-06T18:14:52Z

Revised PR for #11645

wenduwan · 2023-06-06T19:11:45Z

@amirshehataornl Could you hold on for a sec? I have a pending PR to add to this patch series #11728

amirshehataornl · 2023-06-06T20:25:57Z

This PR is meant to carry all the patches for the distances. So it will wait until 11728 lands on master, then I'll include it in the port. This way everything works.

wenduwan · 2023-06-06T20:55:38Z

This PR is meant to carry all the patches for the distances. So it will wait until 11728 lands on master, then I'll include it in the port. This way everything works.

Gotcha. The change was just merged to main - it is needed to maintain backward compatibility for EFA, so I really appreciate if you could include those 2 commits as well.

Or I can do it separately. let me know1

amirshehataornl · 2023-06-06T21:07:31Z

This PR is meant to carry all the patches for the distances. So it will wait until 11728 lands on master, then I'll include it in the port. This way everything works.

Gotcha. The change was just merged to main - it is needed to maintain backward compatibility for EFA, so I really appreciate if you could include those 2 commits as well.

Or I can do it separately. let me know1

done

wenduwan · 2023-06-06T22:11:32Z

opal/mca/common/ofi/common_ofi.c

+    *valin = val;
+    return nearest;
+}
+


Curious why #if HWLOC_API_VERSION < 0x00020000 was removed in this change?

e.g. https://github.com/open-mpi/ompi/blob/main/opal/mca/common/ofi/common_ofi.c#LL608C1-L608C35

Looks like I missed porting
"Don't use hwloc v2 structures unless we have that version or above"
Trying to do that now

wenduwan · 2023-06-07T00:28:46Z

Encountered another bug. Need to add this PR as well #11744

Protect the logic that uses v2 structures from compiling with hwloc v1.x Signed-off-by: Ralph Castain <rhc@pmix.org> (cherry picked from commit c5a8a78)

This patch fixes a bug in device matching. uuid alone is not sufficient to match openfabrics devices, and therefore we need to also match osname. Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 09fd7a1)

Adjust string lengths according to the uuid format Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 2b282c1)

wenduwan · 2023-06-12T17:16:56Z

@amirshehataornl Could you cherry pick these changes? #11744

There are latent bugs in the get package rank function.

This patch fixes a bug in package rank calculation. The peer rank returned via PMIX_LOCAL_PEERS is the global rank. Matching it with local rank was wrong. Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 88077e1)

Fix a double free bug on the error path. This causes a harsh crash instead of returning an error code. Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 002f5b4)

amirshehataornl · 2023-06-12T20:49:31Z

PR #11739 Updated

gpaulsen · 2023-06-13T17:22:20Z

bot:ibm:retest

gpaulsen · 2023-06-13T19:04:33Z

GitHub Action CI / Milestone Pull Request (pull_request_target) had some issues. Trying to retest that:

bot:aws:retest

gpaulsen · 2023-06-20T15:13:51Z

@wenduwan and @amirshehataornl please review, and we'll get it in.

wenduwan · 2023-06-20T15:18:05Z

@gpaulsen Yes I plan to test out the change today/tomo and report back.

wenduwan · 2023-06-20T22:56:35Z

@amirshehataornl Got a compile error with internal (old) hwloc and libevent

make[2]: Entering directory `/home/ec2-user/ompi/opal/mca/timer'
  CC       base/timer_base_open.lo
base/timer_base_open.c: In function 'opal_timer_base_close':
base/timer_base_open.c:60:9: warning: unused variable 'ret' [-Wunused-variable]
     int ret;
         ^~~
  CCLD     libmca_timer.la
make[3]: Entering directory `/home/ec2-user/ompi/opal/mca/timer'
make[3]: Nothing to be done for `install-exec-am'.
make[3]: Leaving directory `/home/ec2-user/ompi/opal/mca/timer'
make[2]: Leaving directory `/home/ec2-user/ompi/opal/mca/timer'
Making install in mca/common/ofi
make[2]: Entering directory `/home/ec2-user/ompi/opal/mca/common/ofi'
  CC       common_ofi.lo
  LN_S     libopen-palmca_common_ofi.la
common_ofi.c: In function 'get_package_rank':
common_ofi.c:782:20: error: 'ranks_on_package' undeclared (first use in this function)
             return ranks_on_package;
                    ^~~~~~~~~~~~~~~~
common_ofi.c:782:20: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [common_ofi.lo] Error 1
make[2]: Leaving directory `/home/ec2-user/ompi/opal/mca/common/ofi'

Need to pick up #11693 as well

wenduwan · 2023-06-20T23:20:25Z

I cherry picked 9046276 and git rebased - ran through a couple 2 nodes tests and the change looks good.

@amirshehataornl Thank you!

awlauria · 2023-06-22T12:55:49Z

Is this ready to merge?

amirshehataornl · 2023-06-22T12:56:47Z

It looks good from my side.

wenduwan · 2023-06-22T13:17:53Z

@gpaulsen I think @amirshehataornl missed 9046276

I will open a PR to cherry-pick that change

wenduwan · 2023-06-22T13:49:50Z

The current_package_rank variable is undefined https://github.com/open-mpi/ompi/blob/v5.0.x/opal/mca/common/ofi/common_ofi.c#L786

amirshehataornl · 2023-06-22T14:11:28Z

The current_package_rank variable is undefined https://github.com/open-mpi/ompi/blob/v5.0.x/opal/mca/common/ofi/common_ofi.c#L786

not sure I follow
It's defined here:

ompi/opal/mca/common/ofi/common_ofi.c

Line 723 in 9717dbf

uint16_t current_package_rank = 0;

wenduwan · 2023-06-22T14:49:04Z

@amirshehataornl Sorry I meant ranks_on_package here

amirshehataornl · 2023-06-22T14:54:29Z

hmmm how did that build in the first place? Are you creating a PR to fix that, or should I?

wenduwan · 2023-06-22T14:56:43Z

Unfortunately Open MPI CI does not build OFI so this was not caught.

I have opened a backport PR #11775

github-actions bot added the Target: v5.0.x label Jun 6, 2023

github-actions bot added this to the v5.0.0 milestone Jun 6, 2023

amirshehataornl force-pushed the v5.0.x branch from 52aa19b to 6626503 Compare June 6, 2023 16:49

amirshehataornl and others added 3 commits June 6, 2023 13:14

Do not compute distances when unbound

1fa904e

Unbound procs cannot have a device distance Signed-off-by: Ralph Castain <rhc@pmix.org> (cherry picked from commit 8a446f6)

amirshehataornl force-pushed the v5.0.x branch from 6626503 to 1fa904e Compare June 6, 2023 17:15

naughtont3 mentioned this pull request Jun 6, 2023

v5.0.x: ofi: NIC selection update #11645

Closed

naughtont3 changed the title ~~V5.0.x~~ V5.0.x: ofi: NIC selection update (revised) Jun 6, 2023

wenduwan reviewed Jun 6, 2023

View reviewed changes

rhc54 and others added 3 commits June 7, 2023 15:50

Don't use hwloc v2 structures unless we have that version or above

4fe862c

Protect the logic that uses v2 structures from compiling with hwloc v1.x Signed-off-by: Ralph Castain <rhc@pmix.org> (cherry picked from commit c5a8a78)

opal/common/ofi: bugfix to string lengths

6ae9248

Adjust string lengths according to the uuid format Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 2b282c1)

amirshehataornl force-pushed the v5.0.x branch from 3344cb0 to 6ae9248 Compare June 7, 2023 19:54

wenduwan added 2 commits June 12, 2023 16:47

opal/mca/ofi: fix a double free

7d878fe

Fix a double free bug on the error path. This causes a harsh crash instead of returning an error code. Signed-off-by: Wenduo Wang <wenduwan@amazon.com> (cherry picked from commit 002f5b4)

gpaulsen approved these changes Jun 22, 2023

View reviewed changes

gpaulsen merged commit 9717dbf into open-mpi:v5.0.x Jun 22, 2023

wenduwan mentioned this pull request Jun 22, 2023

[v5.0.x] opal/ofi: package rank calculation bugfix #11775

Merged

V5.0.x: ofi: NIC selection update (revised) #11739

V5.0.x: ofi: NIC selection update (revised) #11739

Uh oh!

Conversation

amirshehataornl commented Jun 6, 2023

Uh oh!

github-actions bot commented Jun 6, 2023

Uh oh!

github-actions bot commented Jun 6, 2023

Uh oh!

naughtont3 commented Jun 6, 2023

Uh oh!

wenduwan commented Jun 6, 2023

Uh oh!

amirshehataornl commented Jun 6, 2023

Uh oh!

wenduwan commented Jun 6, 2023

Uh oh!

amirshehataornl commented Jun 6, 2023

Uh oh!

wenduwan Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

amirshehataornl Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenduwan commented Jun 7, 2023

Uh oh!

wenduwan commented Jun 12, 2023

Uh oh!

amirshehataornl commented Jun 12, 2023

Uh oh!

gpaulsen commented Jun 13, 2023

Uh oh!

gpaulsen commented Jun 13, 2023

Uh oh!

gpaulsen commented Jun 20, 2023

Uh oh!

wenduwan commented Jun 20, 2023

Uh oh!

wenduwan commented Jun 20, 2023

Uh oh!

wenduwan commented Jun 20, 2023

Uh oh!

awlauria commented Jun 22, 2023

Uh oh!

amirshehataornl commented Jun 22, 2023

Uh oh!

wenduwan commented Jun 22, 2023

Uh oh!

wenduwan commented Jun 22, 2023

Uh oh!

amirshehataornl commented Jun 22, 2023

Uh oh!

wenduwan commented Jun 22, 2023

Uh oh!

amirshehataornl commented Jun 22, 2023

Uh oh!

wenduwan commented Jun 22, 2023

Uh oh!

Uh oh!

amirshehataornl Jun 7, 2023 •

edited

Loading