ch4/ucx: Use UCX active message API #4934

raffenet · 2020-11-19T21:52:44Z

Pull Request Description

Replace layering of active messages over tagged send/recv interface
with UCP active messages. This is an initial, functional
implementation. It should work the same as before, but eliminates a
probe operation in netmod progress that can slowdown applications
significantly when there is a buildup of unexpected messages.

Expected Impact

Author Checklist

raffenet · 2020-11-19T21:53:08Z

test:mpich/ch4/ucx

raffenet · 2020-11-19T21:54:31Z

test:mpich/ch4/ucx

raffenet · 2020-11-19T21:55:37Z

Manual testing is showing an odd failure in alltoall1 and nballtoall1 tests, but otherwise is mostly working. I will continue to investigate.

raffenet · 2020-11-19T21:55:58Z

Manual testing is showing an odd failure in alltoall1 and nballtoall1 tests, but otherwise is mostly working. I will continue to investigate.

I should note this is in the am-only configuration.

hzhou · 2020-11-20T02:05:51Z

I think at some point jenkins server crashed. Rerun-the tests:

test:mpich/ch4/ucx

hzhou

This PR looks fine to me. Just a minor comment.

src/mpid/ch4/netmod/ucx/ucx_init.c

raffenet · 2020-11-20T02:26:50Z

test:mpich/ch4/ucx
test:mpich/warnings

raffenet · 2020-11-20T02:44:31Z

Just for info, the AM APIs were added in UCX 1.7.0.

raffenet · 2020-11-20T02:54:02Z

There also might be a bug when using the AM API and sending to self. Need to see if it consistent with other versions of the library.

raffenet · 2020-11-20T16:37:09Z

test:mpich/ch4/ucx

raffenet · 2020-11-20T16:50:44Z

So this version copies all the data to a temporary buffer before calling the ch4 level callback. That fixes the alignment issues from ubsan, but it adds some significant overhead.

Regardless of how we decide to move forward, this is a useful proof-of-concept to use for my talk later this month. Maybe I can convince them to make AM buffers well-aligned, unlike the libfabric folks and FI_MULTI_RECV...

hzhou · 2020-11-20T16:58:06Z

Regardless of how we decide to move forward, this is a useful proof-of-concept to use for my talk later this month. Maybe I can convince them to make AM buffers well-aligned, unlike the libfabric folks and FI_MULTI_RECV...

Yes! Do that 😃

raffenet · 2020-11-20T18:42:16Z

A new AM API was added recently that is worth some investigation, too. openucx/ucx#5125

They even have an rdnv + am_recv concept that could map well to ch4-level interfaces.

hzhou · 2021-03-02T00:10:38Z

@raffenet Did you tried to talk to the folks in the last ucx meeting? Should we move forward?

Refresh test --
test:mpich/ch4/ucx

raffenet · 2021-03-02T14:32:46Z

They suggested a minimum version check like we do for other libraries. I'll add an autoconf patch to the PR, then we can more forward with it.

raffenet · 2021-03-02T15:18:58Z

test:mpich/ch4/ucx

raffenet · 2021-03-02T15:36:39Z

test:mpich/ch4/ucx

raffenet · 2021-03-02T16:03:38Z

Updated external UCX lib in Jenkins.
test:mpich/ch4/ucx

raffenet · 2021-03-02T19:12:14Z

test:mpich/ch4/ucx

raffenet · 2021-04-07T19:18:54Z

That being said - for performance reasons it would be better to switch to new AM API ucp_worker_set_am_recv_handler, which supports rndv, by passing UCP_AM_RECV_ATTR_FLAG_RNDV in ucp_am_recv_param_t.recv_attr. This means the callback has to call ucp_am_recv_data_nbx in order to receive the message.

I guess you need re-do this PR with the new AM API as they suggested.

Yup. Working on it. It means we'll be tied to the newest version of UCX (released ~30 days ago), but that's probably not a big issue for MPICH 4.0.

hzhou · 2021-04-07T19:37:47Z

It means we'll be tied to the newest version of UCX (released ~30 days ago), but that's probably not a big issue for MPICH 4.0.

Ideally, we can consider both implementations and do a runtime selection. But probably it is not worth the trouble considering we have an embedded version and the default is to pick OFI -- oops, I guess that's not true, but it really makes sense for us to pick OFI as default. So another impact is we need to add more logic in configure that we need to check the versions of external ucx, and when they are too old, we need to use embedded ucx (but still pick ucx as a preference).

raffenet · 2021-04-08T14:13:28Z

Ideally, we can consider both implementations and do a runtime selection.

To be more specific, the UCX AM interfaces didn't actually change. But the way they work changed in between versions WHILE STILL CLAIMING TO BE COMPATIBLE. There is no documentation about the change in semantics. There is no way to know how to deal with the new behavior. The old AM interface is unusable. It would have been better if they just deleted it from the library altogether.

raffenet · 2021-04-12T20:43:25Z

While testing a fix from the UCX folks for the backward compatibility issue, I confirmed there was still a bug in the AM code, which the last patch should hopefully fix.
test:mpich/ch4/most

hzhou

Looks good. A couple of minor comments. I am eager to merge this.

hzhou · 2021-04-12T22:39:01Z

src/mpid/ch4/netmod/ucx/subconfigure.m4

+        AC_MSG_RESULT([$pac_have_ucx])
+
+        # if an old ucx was specified by the user, throw an error
+        if test "$pac_have_ucx" = "no" -a -n "$with_ucx" -a "$with_ucx" != "yes" ; then


Maybe we need to move this check into the "non-embedded" branch later.

I think we always need an error message in this case ($pack_have_ucx = no). For better error messages, we may have to run two tests, one just probe ucx header, the second one check versions and we need tell the user what is the required minimum version.

OK, will do.

You think throw an error when system UCX is found but is not new enough? That is, tell the user to provide embedded rather than fallback automatically? Obviously we want to throw an error if they specify an old installation. I am thinking of the --with-ucx case.

hzhou · 2021-04-12T22:41:50Z

src/mpid/ch4/netmod/ucx/ucx_progress.c

-        (char *) msg_hdr->payload + (info->length - msg_hdr->data_sz - sizeof(*msg_hdr));
+    /* need to copy the message data for alignment purposes */
+    tmp = MPL_malloc(length, MPL_MEM_BUFFER);
+    MPIR_Memcpy(tmp, data, length);


So UCX also doesn't guarantee any minimum alignment?

That's right. With the new API (>= 1.10.0) there is RNDV support for AMs so that we can save an extra copy. I think we can work on integrating that, but it is still quite new so this PR is probably a good starting point for now.

hzhou · 2021-04-12T22:45:55Z

FYI: the ofi asan fix #5210 just merged

raffenet · 2021-04-13T15:39:20Z

test:mpich/ch4/most ✅

raffenet · 2021-04-13T16:43:34Z

src/mpid/ch4/netmod/ucx/subconfigure.m4

+
+        # if a too old UCX was found, throw an error
+        if test "$ucx_happy" = "no" ; then
+            AC_MSG_ERROR([UCXinstallation does not meet minimum version requirement (v1.7.0). Please upgrade your installation, or use --with-ucx=embedded.])


missing a space here 🤦. will fixup before merge if approved.

raffenet · 2021-04-13T16:43:36Z

src/mpid/ch4/netmod/ucx/subconfigure.m4

@@ -46,6 +46,19 @@ AM_COND_IF([BUILD_CH4_NETMOD_UCX],[
    else
        dnl PAC_PROBE_HEADER_LIB must've been successful
        AC_MSG_NOTICE([CH4 UCX Netmod:  Using an external ucx])
+
+        AC_MSG_CHECKING([if UCX meets minimum version requirement])


moved version check to this block, which will throw an error if the external lib is not new enough, regardless of whether the user specified it or not.

hzhou

Looks good to me.

I am going to run a custom asan test to see where we stand.

hzhou · 2021-04-13T17:19:14Z

test:mpich/custom --enable-g=asan
netmod: ch4:ucx
compiler: gcc-9

raffenet · 2021-04-13T18:59:10Z

I am going to run a custom asan test to see where we stand.

Looks like dtpools data corruption + 1 slow test failure. I'm curious what is happening in the dtpools cases. I wonder if disabling the UCX memory hooks will change anything. Will try a quick experiment. Will merge this in the meantime.

Require UCX version 1.7.0 or later. This ensures atomic and active message APIs utilized by MPICH are provided.

Replace layering of active messages over tagged send/recv interface with UCP active messages. This is an initial, functional implementation. It should work the same as before, but eliminates a probe operation in netmod progress that can slowdown applications significantly when there is a buildup of unexpected messages.

We now require UCX v1.7.0 to build MPICH. These RMA API checks from versions < v1.4.0 are no longer needed.

In the case of ch4/ucx self transport, active messages are sent and handled immediately by the target. In the case of rendezvous, it means message data is copied immediately upon CTS issue. Make sure the recv request is fully setup before CTS to avoid unexpected crashes.

raffenet · 2021-04-13T19:00:08Z

@hzhou I also added a commit to note the UCX version requirement in CHANGES.

raffenet · 2021-04-14T14:06:04Z

I am going to run a custom asan test to see where we stand.

Looks like dtpools data corruption + 1 slow test failure. I'm curious what is happening in the dtpools cases. I wonder if disabling the UCX memory hooks will change anything. Will try a quick experiment. Will merge this in the meantime.

I can reproduce the failures with the embedded module. Disabling hooks (UCX_MEM_MMAP_HOOK_MODE=none) does fix them. Oddly enough, I could not reproduce them with an external library. Need to double check versions.

hzhou · 2021-04-14T14:59:46Z

Disabling hooks (UCX_MEM_MMAP_HOOK_MODE=none) does fix them.

👍

Is that an environment variable that we can set for the asan test?

raffenet · 2021-04-14T16:11:41Z

Disabling hooks (UCX_MEM_MMAP_HOOK_MODE=none) does fix them.

👍

Is that an environment variable that we can set for the asan test?

Yes.

raffenet force-pushed the ucx-am branch from d3f3110 to a0b7ae3 Compare November 19, 2020 21:54

hzhou reviewed Nov 20, 2020

View reviewed changes

src/mpid/ch4/netmod/ucx/ucx_init.c Outdated Show resolved Hide resolved

raffenet force-pushed the ucx-am branch from a0b7ae3 to 25a9bf8 Compare November 20, 2020 02:22

raffenet force-pushed the ucx-am branch from 25a9bf8 to aa6fcf2 Compare November 20, 2020 16:36

raffenet force-pushed the ucx-am branch 2 times, most recently from 99505eb to c6567a8 Compare March 2, 2021 15:18

hzhou mentioned this pull request Mar 2, 2021

ch4/ucx: new active message implementation #4713

Closed

10 tasks

raffenet force-pushed the ucx-am branch 2 times, most recently from 9b7aa2f to fac1afc Compare March 2, 2021 15:36

raffenet force-pushed the ucx-am branch 3 times, most recently from 4854d68 to 0e6f071 Compare March 2, 2021 18:48

raffenet force-pushed the ucx-am branch 3 times, most recently from be2ac88 to e76f47d Compare April 12, 2021 20:43

hzhou reviewed Apr 12, 2021

View reviewed changes

raffenet force-pushed the ucx-am branch from e76f47d to 04e5aac Compare April 13, 2021 15:39

raffenet commented Apr 13, 2021

View reviewed changes

hzhou approved these changes Apr 13, 2021

View reviewed changes

raffenet added 6 commits April 13, 2021 13:59

ch4/ucx: Add minimum version check

ec45fa5

Require UCX version 1.7.0 or later. This ensures atomic and active message APIs utilized by MPICH are provided.

ch4/ucx: Remove RMA API ifdefs

5deac5b

We now require UCX v1.7.0 to build MPICH. These RMA API checks from versions < v1.4.0 are no longer needed.

ch4/ucx: Remove unused define

3a7404d

CHANGES: Add note about UCX version requirement

be87d3b

raffenet force-pushed the ucx-am branch from 04e5aac to be87d3b Compare April 13, 2021 18:59

raffenet merged commit 340b256 into pmodels:main Apr 13, 2021

raffenet deleted the ucx-am branch April 13, 2021 19:10

hzhou mentioned this pull request Apr 14, 2021

ch4/ucx: remove unused variable in ucx_progress.h #5216

Merged

4 tasks

ch4/ucx: Use UCX active message API #4934

ch4/ucx: Use UCX active message API #4934

Conversation

raffenet commented Nov 19, 2020 • edited Loading

Pull Request Description

Expected Impact

Author Checklist

raffenet commented Nov 19, 2020

raffenet commented Nov 19, 2020

raffenet commented Nov 19, 2020

raffenet commented Nov 19, 2020

hzhou commented Nov 20, 2020

hzhou left a comment

Choose a reason for hiding this comment

raffenet commented Nov 20, 2020

raffenet commented Nov 20, 2020

raffenet commented Nov 20, 2020

raffenet commented Nov 20, 2020

raffenet commented Nov 20, 2020

hzhou commented Nov 20, 2020

raffenet commented Nov 20, 2020

hzhou commented Mar 2, 2021

raffenet commented Mar 2, 2021

raffenet commented Mar 2, 2021

raffenet commented Mar 2, 2021

raffenet commented Mar 2, 2021

raffenet commented Mar 2, 2021

raffenet commented Apr 7, 2021

hzhou commented Apr 7, 2021 • edited Loading

raffenet commented Apr 8, 2021 • edited Loading

raffenet commented Apr 12, 2021

hzhou left a comment

Choose a reason for hiding this comment

hzhou Apr 12, 2021

Choose a reason for hiding this comment

raffenet Apr 13, 2021

Choose a reason for hiding this comment

raffenet Apr 13, 2021

Choose a reason for hiding this comment

hzhou Apr 12, 2021

Choose a reason for hiding this comment

raffenet Apr 13, 2021

Choose a reason for hiding this comment

hzhou commented Apr 12, 2021

raffenet commented Apr 13, 2021 • edited Loading

raffenet Apr 13, 2021

Choose a reason for hiding this comment

raffenet Apr 13, 2021

Choose a reason for hiding this comment

hzhou left a comment

Choose a reason for hiding this comment

hzhou commented Apr 13, 2021

raffenet commented Apr 13, 2021

raffenet commented Apr 13, 2021

raffenet commented Apr 14, 2021

hzhou commented Apr 14, 2021

raffenet commented Apr 14, 2021

raffenet commented Nov 19, 2020 •

edited

Loading

hzhou commented Apr 7, 2021 •

edited

Loading

raffenet commented Apr 8, 2021 •

edited

Loading

raffenet commented Apr 13, 2021 •

edited

Loading