Ompi: Simple multiprocess mpirun taking > 1 minute on master

[awlauria]

Background information

Reproducer:

$. mpirun -np 2 --host X:1,Y:1 ./hello

If you enable PMIX_DEBUG you'll see it cycling over this:

[f10n18:128686] HASH:FETCH data for key pml.base.2.0 not found
[f10n18:128686] HASH:FETCH rank 1 key pml.base.2.0
[f10n18:128686] HASH:FETCH proc data for rank 5 not found
[f10n18:128686] HASH:FETCH rank 1 key pml.base.2.0
...

It'll take over 1 minute to complete until it gets unwedged trying to fetch the pml base.

What happening is the following:

• If the pml selection logic determines that there > 1 viable pml, it will have __global rank 0__ expose it using PMIx_Put(PMIX_WORLD...).
• At mca_pml_*_add_procs() time, if > 1 pml was determined viable during selection, it will attempt to query its __local rank 0__ to see what pml it used to verify.
• Since only the __global rank 0__ exposed its pml, all ranks outside of the first node will become wedeged in the PMIx_Get() call until it gives up.

• If only one pml is deemed viable (as is the case when using --mca pml ...), all of this is avoided.

• I confirmed that ucx and pml can hit this bug, haven't checked others.

  • to hit with ob1, I manually raised its priority to be higher than ucx.
• Neither will hit it if explicitly asked for at mpirun time (--mca pml ucx/--mca pml ob1). Because there is only one viable pml, neither will attempt to query.

An easy fix that could work for all pmls is to register the pml on the 0th local process on each node. That way all processes on their local node has a reference to query and check the pml being used.

I'm not sure of the performance impact of that, but testing ompi/pmix master with and without the change showed no discernible performance difference on a MPI_Init/Finalize program.

[rhc54]

Something must have been changed - it is supposedly impossible for procs to use more than one PML. I suspect things are broken because someone made a change that violated that rule?

[rhc54]

Clarifying: it is supposed to be impossible for different procs to choose a different PML. Did someone change that?

[awlauria]

I think this check is in place to make sure that doesn't happen. The logic of the check is to return a OMPI_ERR_UNREACH if the rank it queries pml isn't its own.

[rhc54]

We expressly disabled that as it forces a global exchange that should never be necessary. Why did someone turn it back on?

[awlauria]

Not sure. I agree that it shouldn't be possible for a process to select a different pml than another, it seems silly. It might just be a code cleanup exercise to remove this logic.

mca_pml_base_select() keeps track of viable pmls, and at the end finalizes all others. But if it deemed more than 1 as viable, it keeps track of that count and if > 1 requires all ranks to do this check. Checking the git history on it, it's been this way at least 5 years as far as I can tell.

https://github.com/open-mpi/ompi/blob/master/ompi/mca/pml/base/pml_base_select.c#L189

[rhc54]

May well have been, but that just begs the question: what changed that we suddenly are getting more than one viable PML? I suspect that is the key - we didn't used to, and something changed that now makes it possible...and probably should not.

[awlauria]

The viable logic is basically: "if it has an initialization function, it is viable". And that seems to have not changed in a long time, either.

[rhc54]

Well, I guess it is up to you guys. If you want to go back to unscalable launch so you can exchange that value, then go right ahead. Otherwise, you need to do something to remove it.

[awlauria]

Here is the commit where it was added, seems that you added it back in '08:

https://github.com/open-mpi/ompi/commit/3631a60181a3f29e1d1f0f9142683372dfc56a0c#diff-3f40fbff1e79865a7494102f41eb8096

This links a long email chain that I am sifting through with the reasoning behind it.

[bwbarrett]

We're not seeing the 1 second pause, but in digging into a crash in a job with mixed CM / OB1 PML selection due to a configuration issue on one instance, we're pretty sure the code in pml_base_select doesn't do what we (as in the OMPI community) want. I'll bug the person looking at the issue to chime in on this thread, but I think instead of doing something during MPI_INIT, we want a verification step in pml_*_add_procs(). It's unfortunate that we'd delay the error message, but otherwise we end up with a fairly unscalable all to all at startup.

[rhc54]

Agreed - I think the problem here is that things changed such that the PMLs are both finding components under them (e.g., btl's or effectively mtl's) that can work, and so we aren't having PMLs disqualify themselves like we used to do.

[rhc54]

It all might have been there for awhile and is now being exposed (via the pause) due to the correction in direct modex

[awlauria]

Looking through the email chain - this was added to prevent a hang. Some procs would fail to initialize a pml, so it would drop to another pml to avoid a hang. Here's the email that describes what we have now:

https://www.open-mpi.org/community/lists/devel/2008/06/4211.php

I guess the question is, is this reasoning still valid or should we change it?

[awlauria]

@rhc54 yes, it was exposed in the modex change you are referring to. Without that change I don't see the delay.

[bwbarrett]

Two PMLs being selectable is a pretty regular occurrence, since OB1 is almost always selectable. We rely on priorities to choose the "right" one.

The hang was because obviously, those two processes would never talk to each other, and we weren't reflecting errors properly. I think we have three choices:

1) fix the timing issue and leave the unscalable check. 1 minute for a small scale run is insane, but this is an O(N^2) solution.
2) pml_base_select still publishes the selected PML, but instead of checking the PML pml_base_select, we instead check at add_procs. We're already doing an exchange at add_procs, so this shouldn't change complexity over today. We still can pretty-print useful error messages, but not during MPI_INIT.
3) Audit all the error paths for add_procs and emit an error message if a BTL/MTL tries to talk to a process unsuccessfully. This is likely crap-tons of work to get right, and will be forever brittle.

Personally, I think option (2) has a nice enough behavior.

[awlauria]

Regarding 2) @bwbarrett maybe I am not following - but this check (mca_pml_base_pml_check_selected) is already done at add_procs time as far as I can tell. The PMIx_Get() eventually fails, and mca_pml_base_pml_check_selected() will still return OMPI_SUCCESS, and pml_*_add_procs continues on after only printing a debug log message - if that.

[awlauria]

It seems I misspoke in my original message - the check isn't at pml initialization, it's during the add_procs calls. My mistake.

[jjhursey]

I can reproduce the delay @awlauria mentioned. We have a build where both the ob1 and ucx PML's are eligible after the priority selection. You need at least 2 nodes to reproduce (1 node will not).

Either of the following will trigger the issue (first has ucx as the higher priority, the second has ob1 at the higher priority). master is below:

shell$ time mpirun --np 2 --host c712f6n01:1,c712f6n02:1 ./hello_c
Hello, world, I am 0 of 2, (Open MPI v4.1.0a1, package: Open MPI jjhursey@c712f6n01 Distribution, ident: 4.1.0a1, repo rev: v2.x-dev-7502-g207b2671, Unreleased developer copy, 147)
Hello, world, I am 1 of 2, (Open MPI v4.1.0a1, package: Open MPI jjhursey@c712f6n01 Distribution, ident: 4.1.0a1, repo rev: v2.x-dev-7502-g207b2671, Unreleased developer copy, 147)

real    1m1.705s
user    0m0.014s
sys 0m0.000s
shell$ time mpirun --np 2 --mca pml_ob1_priority 70 --host c712f6n01:1,c712f6n02:1 ./hello_c
Hello, world, I am 0 of 2, (Open MPI v4.1.0a1, package: Open MPI jjhursey@c712f6n01 Distribution, ident: 4.1.0a1, repo rev: v2.x-dev-7502-g207b2671, Unreleased developer copy, 147)
Hello, world, I am 1 of 2, (Open MPI v4.1.0a1, package: Open MPI jjhursey@c712f6n01 Distribution, ident: 4.1.0a1, repo rev: v2.x-dev-7502-g207b2671, Unreleased developer copy, 147)

real    1m1.709s
user    0m0.000s
sys 0m0.013s

[awlauria]

Here's a fix that works in my testing if anyone wants to try/comment:

https://github.com/awlauria/ompi/commit/4301aa3aae7f31eef3d77dfb6e8a03c715e04347

All it does is instead of registering the pml in the modex on global rank 0, it does it for local rank 0 on each node. Some components only add local processes at add_procs() time (ucx for example). So this is a change that should work for all pmls and still preserve the same functional check - if that is what we want.

[awlauria]

@jjhursey to add to that - you need to be running with an up-to-date openpmix master. It won't reproduce with say, v3.1.5.

[rhc54]

Only downside is that it doesn't really check much, does it? All you have done is check that the ones on that particular node all chose the same PML - doesn't say anything about what happens on a remote node, which is the place where the mismatch will most likely occur (assuming it happens at all).

[rhc54]

Two PMLs being selectable is a pretty regular occurrence, since OB1 is almost always selectable. We rely on priorities to choose the "right" one.

Guess I don't understand - isn't this just a case of ensuring the procs on all nodes pick the same priority? Why wouldn't that happen, resulting in them all having the same PML? Outside of a configure issue (where the code is effectively built differently on the different nodes), shouldn't we always wind up with the same PML?

In other words - why have a non-scalable path just to check for an error condition that indicates a whacky system?

[awlauria]

Agree that the check is limited in scope, but it is at least checking something now rather than not working at all and timing out.

We could do a gather like operation at the root where rank 0 on each node reports their pml. But, that seems like overkill for something that in most cases is a non-issue.

If we are for getting rid of these checks as not-needed - that sounds good to me too. I can purge it if we agree that the best thing to do is remove this logic.

[awlauria]

This check is technically worthless right now as it is. The check is failing, but returning OMPI_SUCCESS regardless on any non-root node.

We really don't lose anything by not performing it at this point.

[rhc54]

Yeah, as I recall, I was asked to restore it many years ago because someone had a screwed up system and hung instead of getting an error. Back then, we weren't as concerned about launch scaling as it predated "instant on" and async proc construction.

Given that we now have those things, I think @bwbarrett's option 2 is probably the least ugly IF you are going to do anything at all. I personally am still not convinced that we need to add something that punishes the 99+% who do things right just to give a pretty warning to the few that have a screwed up system.

[rhc54]

Just for grins, I tested dmodex on PRRTE master with PMIx master and there is no slowdown/delay in direct modex - runs very fast across two nodes. So I think the problem is either in OMPI or in the way dmodex is being activated in OPAL.

[awlauria]

@rhc54 considering this pml check is broken, probably has been for a while, and nobody has said anything, I'm in favor of removing it rather than trying to needlessly fix it. It'll be better for performance to not have it anyway.

[jjhursey]

I would prefer that we remove the logic per PR #7480 since it is broken and has been for a while.

However, do we want to preserve a debug option that introduces a full modex between the put and get to sync those keys (per suggestion here) as an easy debugging option with a pretty print help message?

The downside is that it would be a code path that not many folks would ever choose to exercise so it would bit rot, and adds a bit more logic to the PML selection. My preference would be 'no' but if someone wants to advocate for it then I can be convinced otherwise.

[bosilca]

The real problem here is in mca_pml_base_pml_check_selected and is the choice of the process to check the modex from. This test was originally designed to work for the case of MPI_COMM_WORLD, where proc[0] in the array of procs used by pml_add_procs was always the rank 0 in MPI_COMM_WORLD and thus the vpid 0 of the job. Thus, in today's context where pml_add_procs can be called with globally inconsistent sets of peers, the check for the modex info of the first proc on the pml_add_procs list is incorrect.

As it has been discussed before, this test is not only critical for correctness, but there are chances it will never be triggered on large scale systems (where there will be no ambiguity on the selection of the PML, and thus the modex_reqd will always be false, short-cutting this check). Thus, most of the cases where this test is beneficial are smaller scale platforms or container-based setups, where most users will take consistency and reliability over 1/10 of second improvement in startup time. Thus, my vote will be to keep this check, and to correct it such that instead of looking for the modex info of procs[0] we look for the modex info of vpid 0.

[rhc54]

So long as that is an option, it should be fine. The problem right now is that this data is not being circulated in a full modex, and thus it forces a direct modex operation where every node has to request this specific key from the host where vpid 0 resides. This doesn't scale.

Making it an option that defaults to "false" resolves the problem and still let's those who are concerned about such things. However, in those cases, we should still force a complete modex to avoid the all-to-one direct modex operation.

The use of containers is separate from small scale - I don't see people using containers solely in small scale use-cases.

[bosilca]

The optionality we are talking about here should not be driven by an MCA, but by an informed decision of the PML because at the end scalability is irrelevant when the startup does not complete. Making this test optional via MCA is as good as removing it: when MPI_Init deadlocks the first thing users will do is not dig into our extensive list of MCA parameters to find a relevant one to enable, but complain on the mailing list.

Why is this information not circulated in a full modex ? Based on what metric a handful of bytes published by a single process is a blocker to scalability ?

[rhc54]

Sigh. You haven't been following this thread, I fear. The issue here occurs when we are doing an _async_ modex. In this case, the data is _not_ circulated as no modex is performed, and thus we hit a direct modex scenario where every proc is requesting this piece of info. If we do a _full_ modex, which is still the default, then the data will be present and the entire issue doesn't occur.

Perhaps the best solution would be to simply link it to the async modex. If someone requests the async modex, then don't ask for the PML data. If they hit a deadlock, then they can turn off the async request and go back to a full modex.

The easiest way to implement that is to simply use the OPAL_MODEX_RECV_OPTIONAL macro. If the full modex was done, then the PML flag will be present and the data will be retrieved. If the full modex was not done, then the PML flag will not be present and the direct modex will not be executed to retrieve it.

[awlauria]

@rhc54 sounds like that will still require an mca option. Or am I misunderstanding?

I agree with @bosilca that if we attach an mca option to this, and anyone does see a problem, the first thing they are going to do is open an issue with us. At that point we can tell them about the option (if we remember it even exists) - or remove it, and tell them to turn on pml_base logging - which is sufficient to debug this problem. In this scenario I personally vote for the later, if it comes to it.

Browsing the code, I don't see a similar check in osc. And don't see it in collectives either - but I may have missed it. So even if the PML is ok there may be very unlikely scenarios where this happens on the other components and we're back at the user opening an issue with us over why their app crashes/hangs.

[rhc54]

sounds like that will still require an mca option. Or am I misunderstanding?

No, it does not require an MCA option to be added. The "optional" macro automatically adds PMIX_OPTIONAL to the call to PMIx_Get. We already have an MCA param to determine if we are doing full vs async modex - I honestly don't recall the default, but I thought we still default to full.

Root cause of this problem is that we cannot agree on the default PML, and there now exists significant overlap in functionality between the options. I see no resolution to the "default" argument as neither side is going to give way and accept the other as being "default". However, it is only in the case of either an unusual system (e.g., heterogeneous fabric) or a broken installation (e.g., differently configured OMPI installations on different nodes) that one would see this problem.

In those rare cases, it isn't that big a deal to answer an issue by pointing someone to the relevant FAQ. Certainly, that is preferable to constantly getting hammered over slow start times when compared with other MPIs.

[bwbarrett]

ok, i'm being dumb, obviously, but if we're in an add_procs situation, there's already going to be a PMIX_Get, right? Looking at the UCX PML, add_procs will call MODEX_RECV(). The MODEX_RECV is in the MTL instead of CM PML, but the effect is the same. The BTLs all call MODEX_RECV in their add_procs as well. So we're calling MODEX_RECV in add_procs today to get the remote endpoint.

So here's my question... If we're calling MODEX_RECV to receive the connection endpoint data, why is calling an additional MODEX_RECV (on the SAME proc address) to get the remote PML name going to be an issue? I can see the problem for instant-on (although I think in instant-on, we can probably hard abort in MPI_INIT), but for discovered-address mode, it seems like it's one additional call that should have almost zero impact on run-time, but allow us to avoid a hang when the network is screwed up (which will happen with more frequency than we like on smaller, not-perfectly-managed systems).

[rhc54]

I wouldn't say "dumb" - you (and others) just aren't reading the entire thread. 😢 Let me try one more time to explain this to folks, though I find it ironic that I am explaining to you folks how your own OMPI code works 😆

Since only the global rank 0 exposed its pml, all ranks outside of the first node will become wedged in the PMIx_Get() call until it gives up.

IF you were indeed looking at the PML selection info for the SAME proc you were already referencing, then _yes_ the info would already be available. But you aren't - you are asking for the PML info from a _specific_ proc that is otherwise not being referenced. This is the root of the problem.

If you want to post the PML info from EVERY proc, then your assessment would be correct. However, you have now increased the size of the modex by a fair amount when aggregated over every proc of even a modest-scale job.

For instant-on, you wouldn't want to do this at all as your MODEX_RECV operations will be locally satisfied but cannot include the PML selection info.

[bwbarrett]

Sorry, wasn't clear. I understand how it works today (and why that's broken).

However, I'm 100% convicted that we MUST have PML validation in the general case. Without PML validation, there is an impossible to debug crash or (worse and more likely) hang when two processes select different PMLs. In the general case, the slightly more bloated modex transfer easily outweighs the pain of this hang. Computing sites with good admins will probably never run into this issue, but my experience is that most of my users don't have good admins :). So now I'm trying to figure out how to have both good scaling for large sites and good error messages for other sites. Here's my thoughts:

1) If, at build time, there is only 1 PML built, then we can skip all the PML verification. This should cover most large installations who custom-build OMPI for their system. It's not perfect, since technically another PML could show up as a dso, but that seems pretty rare. So big, well managed sites get good scaling.

2) If, at run time, we're using instant-on launch, we can skip all the PML verification. This should cover more and more large installations. We should figure out how to add some logic so that if the right component for the instant-on isn't selected, we error or warn or something. That doesn't sound too hard (I'm thinking a PML-level "I've consumed this data" flag), although it does assume that we can generically detect that we're in an instant-on situation.

3) For cases where (1) or (2) isn't true, everyone publishes their PML name. This sucks, but it then means that during add_procs(), we can request the pml for the remote peer and have a clean failure if the PMLs don't match. Way better than a hang, and should not significantly impact runtime of even test applications.

[jsquyres]

I think we're at the point where discussing this on a Webex would be a lot faster than typing it out on Github. I'm going to take a stab at sending a calendar invite to everyone on this issue for tomorrow morning US Pacific time.

[bosilca]

I was playing with this while on the call today. Here is the code that I think does the quick fix we talked about (the one we will use until Amazon provide the real one). It does 2 things, use a MODEX OPTIONAL operation (that I had to implement because PMIx does not provide it for strings), and gets the PML from the rank 0 in MPI_COMM_WORLD (so that everybody has a consistent view).

diff --git a/ompi/mca/pml/base/pml_base_select.c b/ompi/mca/pml/base/pml_base_select.c
index 20cb3010dc..7f8b7c1154 100644
--- a/ompi/mca/pml/base/pml_base_select.c
+++ b/ompi/mca/pml/base/pml_base_select.c
@@ -316,6 +316,46 @@ mca_pml_base_pml_selected(const char *name)
     return rc;
 }

+/**
+ * Provide a simplified macro for retrieving modex data
+ * from another process:
+ *
+ * r - the integer return status from the modex op (int)
+ * s - string key (char*)
+ * p - pointer to the opal_process_name_t of the proc that posted
+ *     the data (opal_process_name_t*)
+ * d - pointer to a location wherein the data object
+ *     it to be returned (char**)
+ * sz - pointer to a location wherein the number of bytes
+ *     in the data object can be returned (size_t)
+ */
+#define OPAL_MODEX_RECV_STRING_OPTIONAL(r, s, p, d, sz)                         \
+    do {                                                                        \
+        pmix_proc_t _proc;                                                      \
+        pmix_value_t *_kv = NULL;                                               \
+        pmix_info_t _info;                                                      \
+        OPAL_OUTPUT_VERBOSE((1, opal_pmix_verbose_output,                       \
+                            "%s[%s:%d] MODEX RECV STRING OPTIONAL FOR PROC %s KEY %s",   \
+                            OPAL_NAME_PRINT(OPAL_PROC_MY_NAME),                 \
+                            __FILE__, __LINE__,                                 \
+                            OPAL_NAME_PRINT(*(p)), (s)));                       \
+        *(d) = NULL;                                                            \
+        *(sz) = 0;                                                              \
+        OPAL_PMIX_CONVERT_NAME(&_proc, (p));                                    \
+        PMIX_INFO_LOAD(&_info, PMIX_OPTIONAL, NULL, PMIX_BOOL);                 \
+        (r) = PMIx_Get(&(_proc), (s), &(_info), 1, &(_kv));                     \
+        if (NULL == _kv) {                                                      \
+            (r) = PMIX_ERR_NOT_FOUND;                                           \
+        } else if (PMIX_SUCCESS == (r)) {                                       \
+            *(d) = (uint8_t*)_kv->data.bo.bytes;                                \
+            *(sz) = _kv->data.bo.size;                                          \
+            _kv->data.bo.bytes = NULL; /* protect the data */                   \
+        }                                                                       \
+        if (NULL != _kv) {                                                      \
+            PMIX_VALUE_RELEASE(_kv);                                            \
+        }                                                                       \
+    } while(0);
+
 int
 mca_pml_base_pml_check_selected(const char *my_pml,
                                 ompi_proc_t **procs,
@@ -324,6 +364,7 @@ mca_pml_base_pml_check_selected(const char *my_pml,
     size_t size;
     int ret;
     char *remote_pml;
+    opal_process_name_t rank0 = {.jobid = ompi_proc_local()->super.proc_name.jobid, .vpid = 0};

     /* if no modex was required by the PML, then
      * we can assume success
@@ -342,8 +383,8 @@ mca_pml_base_pml_check_selected(const char *my_pml,
     }

     /* get the name of the PML module selected by rank=0 */
-    OPAL_MODEX_RECV(ret, &pml_base_component,
-                    &procs[0]->super.proc_name, (void**) &remote_pml, &size);
+    OPAL_MODEX_RECV_STRING_OPTIONAL(ret, mca_base_component_to_string(&pml_base_component),
+                                    &rank0, (void**) &remote_pml, &size);

     /* if this key wasn't found, then just assume all is well... */
     if (OMPI_SUCCESS != ret) {

[awlauria]

Summary of notes from call:

• Agreement that current check is broken.
• PMIx change that exposed this will not be in the 3.x PMIx series - so will not be a problem on OMPI 4.x, but will be for OMPI 5.0.
• Amazon is going to implement a better solution where at add_procs() time, the process will check the PML of the process they are connecting to.
  
  
  
  • For a large scale job, a full modex will be done to compare PML's.
  
  
• In the meantime, a PR is coming shortly to change this to a MODEX_RECV_OPTIONAL, which will hide the slow-down until the above check is implemented in master.
• IBM will implement an mpirun --health-checker option where we can get further info to debug issues in addition to different selected PMLs. This will do something like launch a single process on every target node, and report back if there were any issues. Target a 5.0 release timeframe for initial implementation that can be added to later.

If I left anything out, or got some of the finer details wrong let me know and I will edit this.

[awlauria]

@bosilca thanks for that. Looks good to me. Did you want to pr it? It would probably be better to put the definition of the macro with the others, but since it'll go away with the permanent fix it doesn't matter.

[bosilca]

https://github.com/open-mpi/ompi/pull/7487

[bwbarrett]

As we discussed on the call Friday, marking this a blocker for 5.0, so that we don't launch without some checking that two sides of a connection are using the same PML.

[gpaulsen]

How is this a blocker? We can defer that discussion for a month or more, but It's hard for me to believe this is a blocker.

[gpaulsen]

Sorry for my curt reply above. In the future please provide rational of why a blocker label has been applied.
In today's web-ex, @jsquyres explained that this was discussed as a blocker for Amazon at the face-to-face.
This functionality (auto-selection) has been in at least v3.0,v3.1, and v4.0 (possibly older), however the PMIx_Get call fails at some point. This regression that can result in long hangs in startup is a blocker for use-cases where a PML is not specified by the user.