Add support for GPU buffers for PSM2 MTL #4172

aravindksg · 2017-09-05T16:56:21Z

PSM2 enables support for GPU buffers and CUDA managed memory and it can
directly recognize GPU buffers, handle copies between HFIs and GPUs.
Therefore, it is not required for OMPI to handle GPU buffers for pt2pt cases.
In this patch, we allow the PSM2 MTL to specify when
it does not require CUDA convertor support. This allows us to skip CUDA
convertor init phases and lets PSM2 handle the memory transfers.

This translates to improvements in latency.
The patch enables blocking collectives and workloads with GPU contiguous,
GPU non-contiguous memory.

(cherry picked from commit 2e83cf1)
Signed-off-by: Aravind Gopalakrishnan [email protected]

Conflicts:
opal/datatype/opal_convertor.h

ompiteam-bot · 2017-09-05T16:56:23Z

Can one of the admins verify this patch?

bosilca · 2017-09-05T20:11:36Z

The PR message mentions a conflict on opal/datatype/opal_convertor.h. I don't see any ...

jsquyres · 2017-09-05T20:13:46Z

@artpol84 @Di0gen This looks like the same periodic failure we see from Mellanox Jenkins: it claims that depcomp does not exist. Have you ever been able to track this down?

bot:mellanox:retest

aravindksg · 2017-09-05T20:42:01Z

@bosilca , I had resolved the conflicts before committing the changes. There were two complaints from git:

Regarding one of the copyright lines at the top of the file. (Resolution was to only introduce the relevant copyright line to the patch)
Regarding the macro definition-
#define CONVERTOR_HAS_REMOTE_SIZE 0x20000000

I left out this macro definition to resolve the conflict resulting from the cherry-pick.

bwbarrett · 2017-09-05T20:51:45Z

ok to test

artpol84 · 2017-09-06T08:55:35Z

@jsquyres unfortunately for now we don't know what causes depcomp to be missing. We'll try to keep an eye on this failure.

artpol84 · 2017-09-06T09:01:28Z

But I saw this

20:08:57 ompi/Makefile.am: installing 'config/depcomp'
20:08:58 parallel-tests: installing 'config/test-driver'
20:09:17 configure.ac: installing 'config/ylwrap'
20:09:45 cp: skipping file ‘/hpc/local/share/automake-1.15/depcomp’, as it was replaced while being copied
20:09:56 autoreconf: Leaving directory `.'

Which may be a clue. Will try to investigate.

jsquyres · 2017-09-06T13:38:09Z

@artpol84 Cool. I think we saw this same symptom quite a while ago -- the conjecture at the time was that somehow there were directory trees being re-used by different Jenkins builds. Or somehow the autotools themselves were being re-installed behind the scenes (i.e., outside of Jenkins), which caused this race condition. Good luck.

artpol84 · 2017-09-07T14:24:07Z

@jsquyres this should be fixed now, please let me know if this will be observed again.

jsquyres · 2017-09-07T14:27:04Z

@artpol84 Great, thank you! Just curious (since this went on for so long) -- what was the issue?

Re-run on the Cray to fix the opal_path_nfs test issue:

bot:ompi:retest

artpol84 · 2017-09-07T14:32:42Z

@jsquyres it was infrastructure-related. depcomp was accidentally "updated" quite frequently without need. "cp" was confused by depcomp file that was "modified" during the copy operation, copy was aborted and later missing depcomp was causing a problem.

bwbarrett · 2017-09-07T18:02:53Z

Looks like the Cray went offline during the test...

bot:ompi:retest

aravindksg · 2017-09-25T18:12:28Z

Ping @bosilca : could you please Ack this PR as well? The code has already been reviewed and merged to master and v3.0.x branch (PR's 4143 and 4171)

hppritcha · 2017-09-25T19:18:06Z

The RMs for the 2.1.x release are reluctant to take this feature unless its fixing a correctness bug.

matcabral · 2017-09-25T21:26:58Z

Hi @hppritcha, this is not a bug fix, but a new feature. Without this patch OMPI does not support the GPU Direct functionality in PSM2 library.
Does this mean that new features will ONLY be accepted in 3.x ?
thanks,

matcabral · 2017-09-26T21:56:30Z

Ok, I'm correcting the label to a performance bug and retracting my previous comment about new feature, and here is the rationale: CUDA support is already part of OMPI. Without this fix OMPI built with CUDA support runs significantly slower on a CUDA aware PSM2 library.

jsquyres · 2017-09-27T10:21:37Z

ompi/mca/mtl/psm2/mtl_psm2.c

+    cuda_env = getenv("PSM2_CUDA");
+    if (!cuda_env || ( strcmp(cuda_env, "0") == 0) )
+        opal_output(0, "Warning: If running with device buffers, there is a"
+                    " chance the application might fail. Try setting PSM2_CUDA=1.\n");


Please use opal_show_help() for these kinds of messages (because opal_show_help() deduplicates such messages when outputting to mpirun). There's also very little context given in this message to inform the user that the message is being emitted from the Open MPI PSM2 code.

ok, will fix this

jsquyres · 2017-09-27T10:22:25Z

ompi/mca/mtl/psm2/mtl_psm2.c

+    ompi_mtl_psm2.super.mtl_flags |= MCA_MTL_BASE_FLAG_CUDA_INIT_DISABLE;
+
+    cuda_env = getenv("PSM2_CUDA");
+    if (!cuda_env || ( strcmp(cuda_env, "0") == 0) )


Is PSM2_CUDA an environment variable that is exported by the PSM2 library? Or is that an env variable that is solely being used by Open MPI? If it is solely used by the PSM2 support in Open MPI, it should be an MCA variable.

It is an envvar exported by PSM2 library.

it is actually not "exported", but "read" by PSM2.

jsquyres · 2017-09-27T10:23:37Z

ompi/mca/pml/cm/pml_cm_recvreq.h

    opal_convertor_copy_and_prepare_for_recv(                           \
        ompi_mpi_local_convertor,                                       \
        &(datatype->super),                                             \
        count,                                                          \
        addr,                                                           \
-        0,                                                              \
+        flags,                                                              \


Nit: since the rest of the code here is scrupulously pretty, you might want to re-indent the \ at the end of this line.

jsquyres · 2017-09-27T10:24:01Z

ompi/mca/pml/cm/pml_cm_recvreq.h

@@ -153,6 +158,7 @@ do {                                                                    \
                                          datatype,                     \
                                          addr,                         \
                                          count,                        \
+					  flags,                        \


Ditto with above -- you might want to re-indent flags to match the others (beware of tabs vs. spaces -- Open MPI style requires spaces and bans tabs). This happens a few other places in this PR, too.

Will fix the indentation. Thought I converted tabs to spaces, but I'll double check.

aravindksg · 2017-09-27T18:33:58Z

@jsquyres Since master and v3.0.x branch already pulled these changes in, shall I generate separate PR's to fix the warning message and indentation issues there?

jsquyres · 2017-09-27T18:35:36Z

Do a separate PR for master to make the changes. Once that PR is merged in, you can cherry pick the commit(s) from that master PR to this PR (i.e., just add the commits here -- no need to make a 2nd PR).

Make sense?

aravindksg · 2017-10-04T22:23:38Z

@jsquyres , a patch to address your concerns has been merged to master and cherry-picked here. Could you also please review?

jsquyres · 2017-10-05T14:24:35Z

ompi/mca/mtl/psm2/help-mtl-psm2.txt

@@ -45,3 +45,7 @@ Unknown path record query mechanism %s. Supported mechanisms are %s.
 #
 [message too big]
 Message size %llu bigger than supported by PSM2 API. Max = %llu
+#
+[no psm2 cuda env]
+Using CUDA enabled OpenMPI but PSM2_CUDA environment variable is %s.


Open MPI is two words, not one.

Also, please use full sentences and good grammar in these messages. These are meant to be formal help messages displayed to the user, not "good enough" messages that are only intended for developers. Also, the way you are adding an additional lengthy string via the call to opal_show_help() is... unconventional.

More below, where you actually call opal_show_help().

jsquyres · 2017-10-05T14:29:36Z

ompi/mca/mtl/psm2/mtl_psm2_component.c

+                       "Host buffers,\nthere will be a performance penalty"
+                       " due to OMPI force setting this variable now.\n"
+                       "Set environment variable to 0 if using Host buffers" );
+        setenv("PSM2_CUDA", "1", 0);


Can't this lead to a segv if you put the "1" string into the environment, but then the PSM2 MTL is dlclose()d (because the PSM2 MTL was not selected)?

It seems to be a harmless setting. I tested this by increasing OFI MTL priority to be higher than PSM2 and I did see the environment variable being set. But the application itself worked fine. Also- PSM2_DEVICES envvar setting is now done in component registration phase (PR #3834).

I'm not 100% sure it's harmless. I think you may be leaving now-unallocated memory in the environ array when the PSM MTL is unloaded. It probably won't cause problems, but if anyone goes to try to read it -- especially if that virtual address is no longer valid -- it may cause problems. You might be able to test that a little more rigorously (this is one reason that passing values around in the environment is kinda sucky).

If I'm right, it may be best to strdup() the "1", even though that technically makes a minor memory leak.

And/or you might defer this setenv until you know that PSM2 is going to be used. Then you will have a much smaller window for the bogus memory pointer in environ to cause a problem (i.e., because it won't become bogus until the PSM MTL is closed during MPI_FINALIZE, but that's usually on the way out of the process, anyway).

jsquyres · 2017-10-05T14:47:03Z

ompi/mca/mtl/psm2/mtl_psm2_component.c

+                       "not set",
+                       "Host buffers,\nthere will be a performance penalty"
+                       " due to OMPI force setting this variable now.\n"
+                       "Set environment variable to 0 if using Host buffers" );


Do not use "OMPI" in a show help string -- the product name is "Open MPI" (two words).

I would suggest that you should have 2 different show_help messages here, rather than feeding in a lengthy string to opal_show_help().

Additionally -- this message is being displayed during the component registration phase. Isn't that far too early? I.e., won't it display on systems that don't even have PSM2 and/or CUDA hardware available?

I would think that you should only display these messages if a) the PSM2 MTL is actually selected and b) there are CUDA devices in the system. Otherwise, if this message is emitted if a) the user doesn't have PSM2 hardware or b) doesn't have CUDA devices, the user will (rightfully) be quite confused. For example, you might replace this message with:

Warning: Open MPI has detected both PSM2-based hardware and CUDA hardware, but the PSM2_CUDA environment variable was not set. Open MPI has therefore defaulted to setting PSM2_CUDA=1.

This can leave to a performance penalty, however, if your application uses host buffers. You should set the PSM2_CUDA environment variable to 0 before invoking mpirun to both silence this warning and provide the hint to Open MPI / PSM2 that your application will mainly be using host buffers.

Local hostname: %s

(and include the local hostname in the message, just to let the user know which machine Open MPI is talking about)

You can have a similar message for the 2nd opal_show_help(), below.

Also, keep in mind that the component registration function is really only intended to be used to register MCA vars -- it is not actually intended to do anything in terms of initialization or setup.

If you really really really have to set the PSM2_CUDA environment variable all the way up here in the component registration function, you should include a lengthy comment about why you have to do so. And then you'll need to set some kind of state variable (on the component?) indicating that you do so so that you can emit an appropriate show_help message later (i.e., if/when the PSM2 MTL is selected to be used).

Make sense?

@jsquyres , will fix the help text output in separate commit against master and then cherry pick here.

rhc54 · 2017-10-06T19:09:50Z

FWIW: our normal procedure is to use opal_setenv to set it, mark in a flag that we did so, and then use opal_unsetenv to unset it in the close function. This protects the user from what @jsquyres describes.

jsquyres · 2017-10-06T19:11:21Z

Oh, yes, unsetenving it in the component close function is a much better idea. 😄

PSM2 enables support for GPU buffers and CUDA managed memory and it can directly recognize GPU buffers, handle copies between HFIs and GPUs. Therefore, it is not required for OMPI to handle GPU buffers for pt2pt cases. In this patch, we allow the PSM2 MTL to specify when it does not require CUDA convertor support. This allows us to skip CUDA convertor init phases and lets PSM2 handle the memory transfers. This translates to improvements in latency. The patch enables blocking collectives and workloads with GPU contiguous, GPU non-contiguous memory. (cherry picked from commit 2e83cf1) Signed-off-by: Aravind Gopalakrishnan <[email protected]> Conflicts: opal/datatype/opal_convertor.h

If Open MPI is configured with CUDA, then user also should be using a CUDA build of PSM2 and therefore be setting PSM2_CUDA environment variable to 1 while using CUDA buffers for transfers. If we detect this setting to be missing, force set it. If user wants to use this build for regular (Host buffer) transfers, we allow the option of setting PSM2_CUDA=0, but print a warning message to user that it is not a recommended usage scenario. (cherry picked from commit f8a2b7f) Signed-off-by: Aravind Gopalakrishnan <[email protected]> Conflicts: ompi/mca/mtl/psm2/mtl_psm2_component.c

Signed-off-by: Gilles Gouaillardet <[email protected]> (cherry picked from commit 1daa80d) Conflicts: ompi/mca/mtl/psm2/mtl_psm2_component.c

The messages should be printed only in the event of CUDA builds and in the presence of supporting hardware and when PSM2 MTL has actually been selected for use. To this end, move help text output to component init phase. Also use opal_setenv/unsetenv() for safer setting, unsetting of the environment variable and sanitize the help text message. Signed-off-by: Aravind Gopalakrishnan <[email protected]> (cherry picked from commit bea4503) Conflicts: ompi/mca/mtl/psm2/mtl_psm2_component.c

aravindksg · 2017-10-30T17:50:30Z

@jsquyres : The concerns have been addressed in PR #4323 and cherry-picked here. Please review.

jsquyres · 2017-10-30T20:20:13Z

bot:ompi:retest

jsquyres

I have only read the code; it isn't possible for me to compile or test it.

aravindksg · 2017-10-30T21:01:39Z

@jsquyres : To allay any fears regarding testing: I have compile-tested this and also ran combinations of both CUDA and non-CUDA workloads against the resulting libmpi. They work fine 👍

jsquyres · 2017-10-30T21:21:34Z

@aravindksg I figured. I just wanted to qualify my review. 😄

jsquyres · 2017-10-30T21:22:00Z

@hppritcha I'm good with this PR.

matcabral requested review from bosilca and hjelmn September 5, 2017 16:58

matcabral added this to the v2.0.4 milestone Sep 5, 2017

matcabral added the enhancement label Sep 5, 2017

matcabral modified the milestones: v2.1.3, v2.0.4 Sep 5, 2017

matcabral assigned jsquyres Sep 5, 2017

jsquyres removed their assignment Sep 5, 2017

bwbarrett added the Target: v2.x label Sep 20, 2017

bosilca approved these changes Sep 25, 2017

View reviewed changes

matcabral added performance bug and removed enhancement labels Sep 26, 2017

jsquyres requested changes Sep 27, 2017

View reviewed changes

aravindksg force-pushed the v2.x branch from 34f56d0 to 1a4504f Compare October 2, 2017 19:01

jsquyres requested changes Oct 5, 2017

View reviewed changes

open-mpi deleted a comment from bosilca Oct 5, 2017

aravindksg and others added 3 commits October 27, 2017 09:16

mtl/psm2: plug a memory leak in ompi_mtl_psm2_component_open()

b0dc826

Signed-off-by: Gilles Gouaillardet <[email protected]> (cherry picked from commit 1daa80d) Conflicts: ompi/mca/mtl/psm2/mtl_psm2_component.c

aravindksg force-pushed the v2.x branch from a89b1d1 to 2719f26 Compare October 27, 2017 17:23

jsquyres approved these changes Oct 30, 2017

View reviewed changes

hppritcha merged commit b566ef8 into open-mpi:v2.x Oct 31, 2017

aravindksg deleted the v2.x branch October 31, 2017 23:40

Add support for GPU buffers for PSM2 MTL #4172

Add support for GPU buffers for PSM2 MTL #4172

Uh oh!

Conversation

aravindksg commented Sep 5, 2017

Uh oh!

ompiteam-bot commented Sep 5, 2017

Uh oh!

bosilca commented Sep 5, 2017

Uh oh!

jsquyres commented Sep 5, 2017

Uh oh!

aravindksg commented Sep 5, 2017

Uh oh!

bwbarrett commented Sep 5, 2017

Uh oh!

artpol84 commented Sep 6, 2017

Uh oh!

artpol84 commented Sep 6, 2017

Uh oh!

jsquyres commented Sep 6, 2017

Uh oh!

artpol84 commented Sep 7, 2017

Uh oh!

jsquyres commented Sep 7, 2017

Uh oh!

artpol84 commented Sep 7, 2017

Uh oh!

bwbarrett commented Sep 7, 2017

Uh oh!

aravindksg commented Sep 25, 2017

Uh oh!

hppritcha commented Sep 25, 2017

Uh oh!

matcabral commented Sep 25, 2017

Uh oh!

matcabral commented Sep 26, 2017

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!

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!

aravindksg commented Sep 27, 2017

Uh oh!

jsquyres commented Sep 27, 2017

Uh oh!

aravindksg commented Oct 4, 2017

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!

rhc54 commented Oct 6, 2017

Uh oh!

jsquyres commented Oct 6, 2017

Uh oh!

aravindksg commented Oct 30, 2017

Uh oh!