Fix performance regression caused by enabling opal thread support #1287

hjelmn · 2016-07-28T12:58:49Z

This commit adds opal_using_threads() protection around the atomic
operation in OBJ_RETAIN/OBJ_RELEASE. This resolves the performance
issues seen when running psm with MPI_THREAD_SINGLE.

To avoid issues with header dependencies opal_using_threads() has been
moved to a new header (thread_usage.h). The OPAL_THREAD_ADD* and
OPAL_THREAD_CMPSET* macros have also been relocated to this header.

This was cherry-picked off a fix applied to v1.8 and not master. See
open-mpi/ompi#1902.

(cherry picked from commit ce91307)

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

This commit adds opal_using_threads() protection around the atomic operation in OBJ_RETAIN/OBJ_RELEASE. This resolves the performance issues seen when running psm with MPI_THREAD_SINGLE. To avoid issues with header dependencies opal_using_threads() has been moved to a new header (thread_usage.h). The OPAL_THREAD_ADD* and OPAL_THREAD_CMPSET* macros have also been relocated to this header. This was cherry-picked off a fix applied to v1.8 and not master. See open-mpi/ompi#1902. (cherry picked from commit ce91307) Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

ibm-ompi · 2016-07-28T13:00:32Z

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/ibm-ompi/7f110ab6c94da788f0fed01d83c12215

ibm-ompi · 2016-07-28T13:01:21Z

Build Failed with GNU compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/ibm-ompi/514bb6fe123a40de30f56e64a600d85b

mellanox-github · 2016-07-28T13:46:40Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1989/ for details.

hjelmn · 2016-07-28T14:43:01Z

:bot:milestone:v2.0.1
:bot🏷️bug
:bot:assign: @nysal

This fix was lost last year before the 2.x branch.

ompiteam-bot · 2016-07-28T14:43:04Z

Something has gone wrong (error 422).

@jsquyres Please have a look at it!

hjelmn · 2016-07-28T14:43:29Z

@nysal. Please review.

hjelmn · 2016-07-28T14:43:43Z

:bot:milestone:v2.0.1
:bot🏷️bug

hjelmn · 2016-07-28T14:46:06Z

:bot:assign: @nysal

nysal · 2016-07-29T09:22:26Z

opal/threads/thread_usage.h

+}
+#else
+static inline int32_t
+OPAL_THREAD_ADD32(volatile int32_t *addr, int delta)


@hjelmn do we need two versions of OPAL_THREAD_ADD32, OPAL_THREAD_ADD64 and OPAL_THREAD_ADD_SIZE_T. Wouldn't the compiler remove the dead code if opal_using_threads() returns false?

hjelmn · 2016-07-29T12:51:04Z

I added the cleanup commit to this PR due to a bug in OPAL_THREAD_ADD64. The argument was of type int when it should have been int64_t.

lanl-ompi · 2016-07-29T12:56:40Z

Test FAILed.

lanl-ompi · 2016-07-29T12:57:31Z

Test FAILed.

ibm-ompi · 2016-07-29T12:59:54Z

Build Failed with GNU compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/431e65fab860a6cb7d4a3438cc46aaeb

mellanox-github · 2016-07-29T13:00:09Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1995/ for details.

lanl-ompi · 2016-07-29T13:02:21Z

Test FAILed.

This commit expands the OPAL_THREAD macros to include 32- and 64-bit atomic swap. Additionally, macro declararations have been updated to include both OPAL_THREAD_* and OPAL_ATOMIC_*. Before this commit the former was used with add and the later with cmpset. This commit remove the OMPI_THREAD_MULTIPLE macros around the OPAL_THREAD_* function. This should not make a difference in performance as opal_uses_threads is #defined to false when OMPI_THREAD_MULTIPLE is 0. The compiler should optimize out the check on opal_using_threads. Signed-off-by: Nathan Hjelm <hjelmn@me.com> (cherry picked from commit aac6112) Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

ibm-ompi · 2016-07-29T13:06:06Z

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/cbddb0dcc3fb86f444f37d60861b46c4

nysal · 2016-07-29T13:14:50Z

The XL compiler build failed with:

In file included from dss/dss_copy.c:24:
In file included from ../opal/util/output.h:70:
In file included from ../opal/class/opal_object.h:128:
../opal/threads/thread_usage.h:144:1: warning: implicit declaration of function 'OPAL_UNLIKELY' is invalid in C99 [-Wimplicit-function-declaration]
OPAL_THREAD_DEFINE_ATOMIC_ADD(int32_t, 32)
^
../opal/threads/thread_usage.h:107:9: note: expanded from macro 'OPAL_THREAD_DEFINE_ATOMIC_ADD'
    if (OPAL_UNLIKELY(opal_using_threads())) {                          \
        ^
1 warning generated.
1 warning generated.

The opal/prefetch.h header is probably not included

hjelmn · 2016-07-29T13:35:13Z

@nysal Yeah. Fixed that. I brought over one more commit because OPAL_THREAD_ADD64 and OPAL_THREAD_ADD_SIZE_T have the wrong argument type for delta. Been wrong for years. Figured we should go ahead and fix it now.

mellanox-github · 2016-07-29T13:49:16Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1996/ for details.

nysal · 2016-07-29T14:16:20Z

opal/threads/thread_usage.h

+
+#include "opal/sys/atomic.h"
+#include "opal/prefetch.h"
+#include "opal_config.h"


Should we move "#include "opal_config.h"" to the top, although "opal/sys/atomic.h" does include it.

opal_config.h should always be the first header file included in any file.

Figured it was pointless to include since it is included through atomic.h. Will update.

Should you add a #if !defined(...) kind of check at the top to guarantee that this file is only included via atomic.h?

nysal · 2016-08-03T13:48:39Z

With the previous comment about "opal_config.h" addressed, it looks good to me 👍

jsquyres · 2016-08-03T14:44:55Z

@hppritcha Good to go

hjelmn force-pushed the v2.x_atomic branch from 20409c8 to 1c3965b Compare July 28, 2016 12:59

ompiteam-bot added this to the v2.0.1 milestone Jul 28, 2016

ompiteam-bot added the bug label Jul 28, 2016

jsquyres assigned jsquyres and unassigned jsquyres Jul 28, 2016

ompiteam-bot assigned nysal Jul 28, 2016

nysal reviewed Jul 29, 2016
View reviewed changes

hjelmn force-pushed the v2.x_atomic branch from b397cd8 to 170161e Compare July 29, 2016 13:03

nysal reviewed Jul 29, 2016
View reviewed changes

jsquyres added the blocker label Aug 2, 2016

jsquyres mentioned this pull request Aug 2, 2016

Disable atomics in OBJ_RETAIN/RELEASE during THREAD_SINGLE open-mpi/ompi#1902

Closed

ompiteam-bot added the reviewed label Aug 3, 2016

jsquyres added the RM-Approved label Aug 3, 2016

hppritcha merged commit 00e9277 into open-mpi:v2.x Aug 3, 2016

jsquyres mentioned this pull request Aug 5, 2016

users reporting poor sm/vader bandwidth for 2.0.0 when openib BTL active open-mpi/ompi#1943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix performance regression caused by enabling opal thread support #1287

Fix performance regression caused by enabling opal thread support #1287

hjelmn commented Jul 28, 2016

ibm-ompi commented Jul 28, 2016

ibm-ompi commented Jul 28, 2016

mellanox-github commented Jul 28, 2016

hjelmn commented Jul 28, 2016

ompiteam-bot commented Jul 28, 2016

hjelmn commented Jul 28, 2016

hjelmn commented Jul 28, 2016

hjelmn commented Jul 28, 2016

nysal Jul 29, 2016

hjelmn commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

ibm-ompi commented Jul 29, 2016

mellanox-github commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

ibm-ompi commented Jul 29, 2016

nysal commented Jul 29, 2016

hjelmn commented Jul 29, 2016

mellanox-github commented Jul 29, 2016

nysal Jul 29, 2016

jsquyres Jul 29, 2016

hjelmn Jul 29, 2016

jsquyres Jul 29, 2016

nysal commented Aug 3, 2016

jsquyres commented Aug 3, 2016

Fix performance regression caused by enabling opal thread support #1287

Fix performance regression caused by enabling opal thread support #1287

Conversation

hjelmn commented Jul 28, 2016

ibm-ompi commented Jul 28, 2016

ibm-ompi commented Jul 28, 2016

mellanox-github commented Jul 28, 2016

hjelmn commented Jul 28, 2016

ompiteam-bot commented Jul 28, 2016

hjelmn commented Jul 28, 2016

hjelmn commented Jul 28, 2016

hjelmn commented Jul 28, 2016

nysal Jul 29, 2016

Choose a reason for hiding this comment

hjelmn commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

ibm-ompi commented Jul 29, 2016

mellanox-github commented Jul 29, 2016

lanl-ompi commented Jul 29, 2016

ibm-ompi commented Jul 29, 2016

nysal commented Jul 29, 2016

hjelmn commented Jul 29, 2016

mellanox-github commented Jul 29, 2016

nysal Jul 29, 2016

Choose a reason for hiding this comment

jsquyres Jul 29, 2016

Choose a reason for hiding this comment

hjelmn Jul 29, 2016

Choose a reason for hiding this comment

jsquyres Jul 29, 2016

Choose a reason for hiding this comment

nysal commented Aug 3, 2016

jsquyres commented Aug 3, 2016