btl/base: add subsystem to support for active-message based RDMA/atomic emulation #8493

hjelmn · 2021-02-17T16:22:24Z

No description provided.

hjelmn · 2021-02-17T16:31:05Z

Needed to provide working RMA when no RDMA-capable device is present.

awlauria · 2021-02-20T18:54:06Z

bot:aws:retest

awlauria · 2021-02-20T18:59:46Z

Is this series of commits intended to fix #7830 ?

devreal

Looks good to me, except for a few nit-picks.

ompi/mca/osc/rdma/osc_rdma.h

opal/mca/btl/base/btl_base_am_rdma.c

devreal · 2021-02-22T20:45:19Z

opal/mca/btl/base/btl_base_am_rdma.c

+        if (NULL == operation) {
+            /* couldn't even allocate a small amount of memory. not much else can be done. */
+            BTL_ERROR(("could not allocate memory to queue active-message RDMA operation"));
+            abort();


Is there a more graceful way then to call abort here?

Yes, please do not call abort() -- that can lead to terrabytes of corefiles. ☹️

It is either abort or SEGV. IMHO abort is much more graceful. If we get here things are bad and we will abort or SEGV somewhere if we can't even allocate 64 bytes.

abort() drops a corefile -- that's kinda anti-social.

If you need to exit, please call the btl error function.

Already called. In that case abort is not even reached.

Wait. BTL_ERROR doesn't exit so we still want it to exit here. It will SEGV which will dump core files.

So, I can remove the aborts() but the semantics of this error will not change. It will print the error message then crash when operation is dereferenced.

I'm not sure I agree with @jsquyres on the abort thing; I think this is precisely when we want a core file (and, I believe, we call abort elsewhere). This is a should "never, ever happen" type thing, not a "we got our configuration wrong" type thing.

Maybe we should have an option for dropping corefiles. It is very anti-social to have an MTT run fill up filesystems with terrabytes of corefiles.

Shouldn't happen ever in MTT until you are expecting to completely run out of memory. But, again, in that case we really can't control whether you core or not. Up to your shell to set ulimit -c 0.

opal/mca/btl/base/btl_base_am_rdma.c

ompi/mca/osc/rdma/osc_rdma.h

awlauria · 2021-03-03T16:51:37Z

I resolved the conflict in the copyright. Let's move to get this in/verified.

devreal

Thanks for the fixes, LGTM

bwbarrett

we should rebase the branch and fix up the conflicts, not create another merge commit. @awlauria please undo this.

awlauria · 2021-03-03T17:20:50Z

@bwbarrett I agree. However this pr has been open for weeks, I did it for expediency.

If we want this rebased, please ping @hjelmn to do so in a timely manner.

bwbarrett

approving, although the header issue makes me really nervous.

opal/mca/btl/base/btl_base_am_rdma.c

opal/mca/btl/sm/btl_sm_get.c

opal/mca/btl/tcp/btl_tcp_endpoint.c

opal/mca/btl/base/btl_base_am_rdma.c

This commit adds a new subsystem to the btl base: active-message RDMA. The subsystem can be activated on any non-RDMA or partial- RDMA btl (for example tcp) by calling: ```c mca_btl_base_am_rdma_init(mca_btl_base_module_t *btl). ``` Once called the btl module supplied with be augmented to have btl_get, btl_put, btl_fop, and btl_cswap functions. Flags will be set to indicate that the BTL is using active-message versions of these functions. Calling this function on a btl will register a progress function for handling retries of intiator or target-side operations. There may be some overhead once the progress function is registered. Active-message "RDMA" support is not activated on any BTL module by default. Signed-off-by: Nathan Hjelm <hjelmn@google.com>

Signed-off-by: Nathan Hjelm <hjelmn@google.com>

This commit updates osc/rdma to support using alternate BTLs when a primary BTL is not available. There may be at most two alternate BTLs in use at any time. The default is selected to cover shared memory (sm) and off-node (tcp). The priority of osc/rdma is a bit lower when using a set of alternate btls. This will allow another osc component to win if there is an alternative. Signed-off-by: Nathan Hjelm <hjelmn@google.com>

This removes the emulated RDMA code provided as a fallback so btl/sm could be used for osc/rdma. This functionality is now provided by ```btl_base_am_rdma_init ()``` so the dedicated version in btl/sm is no longer needed. Signed-off-by: Nathan Hjelm <hjelmn@google.com>

awlauria · 2021-03-03T21:18:39Z

bot:aws:retest

gpaulsen · 2021-03-04T13:48:07Z

ubuntu executor died with a java exception.

gpaulsen · 2021-03-04T13:48:18Z

bot:aws:retest

jsquyres · 2021-03-04T14:50:15Z

I still see 6 uses of abort() on this PR, and I don't know if I agree with all of them. It looks like some of the uses of abort() in this PR are really assert-like checks -- i.e., "if we got here, that's an OMPI programmer error". Other abort()s are set for run-time errors outside the scope of Open MPI, such as network packet corruption. But a corefile on just the receiver (for example) without a corresponding corefile on the sender -- where the assert-like-check won't fail -- may not reveal anything useful. They'll just take up space on the filesystem.

Outside the scope of this PR, I wonder if we should make opal_assert() that would behave exactly like assert(), but it has the option to drop a corefile or not (e.g., via MCA param or somesuch).

I'm really sensitive to this whole corefile issue because there have been many times over the years where MTT caused the shared filesystem I use for Cisco MTT to fill up with terrabytes/petabytes/whateverbytes of corefiles. My colleagues get (rightfully) pissed at me when this happens.

It could be helpful if one could optionally turn on corefiles from opal_assert() because this would let me leave ulimit -c unlimited to catch the random failures where a corefile can definitely be helpful, but are far harder to isolate and reproduce. If we get large-scale opal_assert() failures and a corefile is needed for further analysis, it would seem statistically probable that running again with core dumps enabled would yield what is needed.

bwbarrett · 2021-03-04T16:40:19Z

Nathan's right; there's not a great way for OMPI to decide whether to drop a core file or not. That's really a system setting (via ulimit). We use abort() or assert() or similar so liberally in the OMPI code base, that this seems like a really weird place to hold the line.

rhc54 · 2021-03-06T23:06:55Z

Given that Cisco isn't running MTT and the folks who are running MTT approve this PR, let's separate the question of what to do with abort (dealing with it in #8557) and get this in so MTT quits failing for those who are running it.

jsquyres · 2021-03-08T16:38:16Z

Given that Cisco isn't running MTT and the folks who are running MTT approve this PR, let's separate the question of what to do with abort (dealing with it in #8557) and get this in so MTT quits failing for those who are running it.

Point of fact: Cisco is running MTT, but it's currently in "trial" mode (i.e., results are not visible by default) because I had some issues with networking and equipment being powered off. Also, I objected to the use of abort() on this PR for two reasons, not just the "filling up of filesystems" reason. I'll reply more on #8557.

hjelmn added feature request Target: main labels Feb 17, 2021

hjelmn added this to the v5.0.0 milestone Feb 17, 2021

devreal requested changes Feb 22, 2021

View reviewed changes

gpaulsen added bug Target: v5.0.x and removed Target: v5.0.x labels Mar 2, 2021

hjelmn force-pushed the provide_support_for_atomics_in_all_btls_because_this_support_is_needed_to_truely_eliminate_the_awful_osc_pt2pt_component_which_has_gone_away_on_the_master_branch branch 3 times, most recently from 1dbe39d to 1fa152d Compare March 2, 2021 17:59

devreal approved these changes Mar 3, 2021

View reviewed changes

rhc54 requested a review from bwbarrett March 3, 2021 17:08

bwbarrett requested changes Mar 3, 2021

View reviewed changes

hjelmn force-pushed the provide_support_for_atomics_in_all_btls_because_this_support_is_needed_to_truely_eliminate_the_awful_osc_pt2pt_component_which_has_gone_away_on_the_master_branch branch from 6010c00 to 59de860 Compare March 3, 2021 17:34

bwbarrett approved these changes Mar 3, 2021

View reviewed changes

hjelmn force-pushed the provide_support_for_atomics_in_all_btls_because_this_support_is_needed_to_truely_eliminate_the_awful_osc_pt2pt_component_which_has_gone_away_on_the_master_branch branch 2 times, most recently from f0100b1 to d318916 Compare March 3, 2021 20:03

bwbarrett approved these changes Mar 3, 2021

View reviewed changes

hjelmn added 4 commits March 3, 2021 13:06

btl/tcp: do not call frag callback if it does not exist

f71381c

Signed-off-by: Nathan Hjelm <hjelmn@google.com>

hjelmn force-pushed the provide_support_for_atomics_in_all_btls_because_this_support_is_needed_to_truely_eliminate_the_awful_osc_pt2pt_component_which_has_gone_away_on_the_master_branch branch from d318916 to 214f6a6 Compare March 3, 2021 20:08

awlauria mentioned this pull request Mar 6, 2021

Add OPAL_ASSERT() macro to wrap assert(). #8557

Closed

rhc54 merged commit 1049307 into open-mpi:master Mar 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

btl/base: add subsystem to support for active-message based RDMA/atomic emulation #8493

btl/base: add subsystem to support for active-message based RDMA/atomic emulation #8493

hjelmn commented Feb 17, 2021

hjelmn commented Feb 17, 2021 •

edited

Loading

awlauria commented Feb 20, 2021

awlauria commented Feb 20, 2021

devreal left a comment

devreal Feb 22, 2021

jsquyres Feb 23, 2021

hjelmn Mar 2, 2021

jsquyres Mar 2, 2021

hjelmn Mar 2, 2021

hjelmn Mar 2, 2021

hjelmn Mar 2, 2021

bwbarrett Mar 3, 2021

jsquyres Mar 3, 2021

hjelmn Mar 3, 2021

awlauria commented Mar 3, 2021

devreal left a comment

bwbarrett left a comment

awlauria commented Mar 3, 2021

bwbarrett left a comment

awlauria commented Mar 3, 2021

gpaulsen commented Mar 4, 2021

gpaulsen commented Mar 4, 2021

jsquyres commented Mar 4, 2021

bwbarrett commented Mar 4, 2021

rhc54 commented Mar 6, 2021

jsquyres commented Mar 8, 2021

btl/base: add subsystem to support for active-message based RDMA/atomic emulation #8493

btl/base: add subsystem to support for active-message based RDMA/atomic emulation #8493

Conversation

hjelmn commented Feb 17, 2021

hjelmn commented Feb 17, 2021 • edited Loading

awlauria commented Feb 20, 2021

awlauria commented Feb 20, 2021

devreal left a 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

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

awlauria commented Mar 3, 2021

devreal left a comment

Choose a reason for hiding this comment

bwbarrett left a comment

Choose a reason for hiding this comment

awlauria commented Mar 3, 2021

bwbarrett left a comment

Choose a reason for hiding this comment

awlauria commented Mar 3, 2021

gpaulsen commented Mar 4, 2021

gpaulsen commented Mar 4, 2021

jsquyres commented Mar 4, 2021

bwbarrett commented Mar 4, 2021

rhc54 commented Mar 6, 2021

jsquyres commented Mar 8, 2021

hjelmn commented Feb 17, 2021 •

edited

Loading