Signal operation type for put with signal #929

wrrobin · 2020-02-06T19:57:38Z

No description provided.

Signed-off-by: Md <[email protected]>

jdinan · 2020-02-06T21:48:33Z

src/shr_transport.h4

+        shmem_transport_xpmem_get(&old_signal_val, sig_addr, sizeof(uint64_t), pe,
+                                  shmem_internal_get_shr_rank(pe));
+        signal += old_signal_val;
+    }


This needs to be an atomic operation, use shmem_shr_transport_atomic.

@jdinan shmem_shr_transport_atomic is enabled through USE_SHR_ATOMICS. Unless we use --enable-shr-atomics flag, we will not be able to use this API, right? Please let me know if I am missing something.

If the target is waiting for several messages using the same signal variable, this will not work, right?

Right, you would need to write code to support builds with and without shared memory atomics.

Thanks @jdinan. One question, from the current code, there seems to be no relation between the flags USE_SHR_ATOMICS and USE_XPMEM/USE_CMA. Was there any particular reason behind this? I am thinking shouldn't USE_SHR_ATOMICS be defined when either of these on-node transports is defined?

For nearly all builds, we can't enable shared memory atomics because the networking layer is not coherent with processor atomics. By default, we only enable shared memory copy to implement put/get. This is why shared memory atomics are a separate option, and disabled by default. CMA is essentially memory copy performed by the Linux kernel and it cannot support atomics.

For CMA, we should directly use the transport layer atomics then, as you also suggested later. But, for XPMEM, can we say that enabling USE_XPMEM should enable USE_SHR_ATOMICS as well? We can use the shared memory atomics if shmem_internal_get_shr_rank returns something other than -1, right? But, currently, shmem_shr_transport_use_atomic also seems to be guarded by USE_SHR_ATOMICS.

Correct, it is always safe to use shared memory put/get because OpenSHMEM says overlapping operations (e.g. between local shared memory and remote network transport accesses) leads to undefined behavior. Atomics do allow this overlapping. Therefore, it is only safe to use shared memory atomics when you know the transport layer is coherent with processor atomics. Because of this, enabling XPMEM only enables shared memory put/get and there is an additional flag to enable shared memory atomics.

jdinan · 2020-02-06T21:49:54Z

src/shr_transport.h4

+        shmem_transport_cma_get(&old_signal_val, sig_addr, sizeof(uint64_t), pe,
+                                shmem_internal_get_shr_rank(pe));
+        signal += old_signal_val;
+    }


This needs to be an atomic. CMA does not support atomics; use shmem_transport_atomic.

Signed-off-by: Md <[email protected]>

wrrobin · 2020-02-12T17:58:52Z

@jdinan Can you please review the changes in shr_transport? There is one test failing in Travis that I am taking a look, but looks like it is related to the unit test change.

jdinan · 2020-02-12T18:32:33Z

src/shr_transport.h4

+#  define ONLY_XPMEM_TRANSPORT 1
+#else
+#  define ONLY_XPMEM_TRANSPORT 0
+#endif


Oh dear, no. This will cause shared memory atomics to be enabled in some paths and not others. You should only need to check USE_SHR_ATOMICS. This flag is set by the configure script. If you are concerned about it being set incorrectly, please resolve those concerns at configure time, not here.

For example, shmem_wait will not be using the correct atomics if USE_SHR_ATOMICS is not set.

Ok. I was thinking initially to use USE_SHR_ATOMICS by defining it when only XPMEM is enabled in the configure script. But for some reason, I misunderstood that we wanted a separate flag.

jdinan · 2020-02-12T18:36:16Z

src/shr_transport.h4

+                              shmem_internal_get_shr_rank(pe));
+    shmem_internal_membar_acq_rel(); /* Memory fence to ensure target PE observes
+                                        stores in the correct order */
+    if (ONLY_XPMEM_TRANSPORT) {


This should be replaced by #if USE_SHR_ATOMICS but other than change this looks looks ok.

…nabled Signed-off-by: Md <[email protected]>

jdinan · 2020-02-12T20:36:18Z

The put_signal_nbi test is failing in the Travis row with huge pages and OFI fence enabled. Huge page support is auto-disabling, looks like possibly the issue is related to the OFI fence flag?

[0000] WARN:  ../../src/symmetric_heap_c.c:187: mmap_alloc
[0000]        file open failed, cannot use huge pages
         0: target[1] = 0 not matching 1
         0: target[2] = 0 not matching 2
         0: target[3] = 0 not matching 3
         0: target[4] = 0 not matching 4
         0: target[5] = 0 not matching 5
         0: target[6] = 0 not matching 6
         0: target[7] = 0 not matching 7
         0: target[8] = 0 not matching 8
         0: target[9] = 0 not matching 9
FAIL put_signal_nbi (exit status: 9)

wrrobin · 2020-02-12T20:44:28Z

Yes, I am trying to reproduce the issue locally. The test is modified with this PR to include a check for ADD signal atomic. Earlier, with no signal op argument, this test was not failing.

jdinan · 2020-02-12T20:47:57Z

configure.ac

@@ -414,6 +414,10 @@ AS_IF([test "$transport_xpmem" = "yes" -o "$transport_cma" = "yes"],
       AC_DEFINE([ENABLE_HARD_POLLING], [1], [Enable hard polling])
      ])

+if test "$enable_shr_atomics" = "no" -a "$transport_xpmem" = "yes" -a "$transport_ofi" = "no" -a "$transport_portals4" = "no" ; then
+    AC_DEFINE([USE_SHR_ATOMICS], [1], [If defined, the shared memory layer will perform processor atomics.])
+fi


We have always treated the above as an invalid build; the configure script should emit a warning any time there isn't a transport selected. That having been said, this seems like a good change. You might want to update the description of the --enable-shr-atomics to say "default: auto" rather than "default: disabled".

A minor fix here, the check for "$enable_shr_atomics" = "no" is not quite right (see Example 1.1 here). It's counterintuitive, but I think this check should be "$enable_shr_atomics" != "no" i.e. "not disabled".

src/shmem_internal.h

Signed-off-by: Md <[email protected]>

jdinan

@wrrobin Based on earlier discussion, these changes look good to me. I unfortunately won't have time to review the unit test, perhaps @davidozog can help there? One last suggestion, try configuring with "--disable-shr-atomics" and "--enable-shr-atomics" and look in src/config.h under your build directory to ensure that the preprocessor macro is being defined correctly (I never trust myself to get the inverted logic of configure parameters correct).

davidozog · 2020-02-13T22:13:33Z

@jdinan, do you have any opinion as to whether this should be included in the v1.4.5 release? All testing would have to be redone, so @wrrobin and I have tentatively decided to defer this merge.

jdinan · 2020-02-13T22:30:58Z

Our CI testing is pretty thorough, apart from Portals, and this is an extension, so I wouldn't be opposed if you feel that it's ready. Since this PR includes an API change to bring the signaling API into compliance with the latest OpenSHMEM 1.5 draft, it might be nice to include.

wrrobin · 2020-02-14T07:16:40Z

@jdinan I have tried with different combinations of --enable/disable-shr-atomics and with-xpmem/ofi and here are the results with the current configure script:

--enable-shr-atomics -> USE_SHR_ATOMICS defined
--disable-shr-atomics -> undefined
--enable-shr-atomics --with-xpmem -> USE_SHR_ATOMICS defined
--disable-shr-atomics --with-xpmem -> USE_SHR_ATOMICS defined
--with-xpmem -> undefined
--with-xpmem --with-ofi -> undefined
--with-xpmem --with-ofi --enable-shr-atomics -> USE_SHR_ATOMICS defined
--with-xpmem --with-ofi --disable-shr-atomics -> undefined

IIUC, the bold italic ones (4,5,7) above are producing incorrect configurations. Please let me know if you think so as well. For 7, I think it is producing the config that the user wants. But, I am not sure whether we can label the config as "auto" rather than "disabled".

If I keep the configure script condition as before

if test "$enable_shr_atomics" != "yes" -a "$transport_xpmem" = "yes" -a "$transport_ofi" = "no" -a "$transport_portals4" = "no" ; then
    AC_DEFINE([USE_SHR_ATOMICS], [1], [If defined, the shared memory layer will perform processor atomics.])
fi

then only the following result changes:
5. --with-xpmem -> USE_SHR_ATOMICS defined

which is the behavior we want to see. The 4th and 7th use-cases are still problematic.

jdinan · 2020-02-14T16:17:29Z

With your current code:

if test "$enable_shr_atomics" != "no" -a "$transport_xpmem" = "yes" -a "$transport_ofi" = "no" -a "$transport_portals4" = "no" ; then
    AC_DEFINE([USE_SHR_ATOMICS], [1], [If defined, the shared memory layer will perform processor atomics.])
fi

For 4, I see:

$ ./configure --disable-shr-atomics --with-xpmem=$HOME/opt/xpmem --enable-pmi-simple
...
$ grep USE_SHR src/config.h
/* #undef USE_SHR_ATOMICS */

For 5:

$ ./configure --with-xpmem=$HOME/opt/xpmem --enable-pmi-simple
...
$ grep USE_SHR src/config.h
#define USE_SHR_ATOMICS 1

That's the right behavior for 7. User got what they asked for. Can you please re-check? The above look like the correct behavior.

Another possibility. There is a portability issue with string comparison in the test utility, which is why you often see if test "x$enable_shr_atomics" != "xno" .... I don't think Linux has this issue, which is why we have been lazy about using this style. You could try that if you are seeing weird behavior.

wrrobin · 2020-02-14T16:41:14Z

I am not sure why I have seen those behaviors before. I am seeing the correct configuration now similar to yours. I am testing these on Cori and not sure now whether the behavior is random.

wrrobin · 2020-02-14T18:50:56Z

@davidozog Can you please review the PR? If we do not see any other issues, we can merge this for the release.

davidozog

Looks good! Just a few comments/nit-picks.

src/transport_portals4.h

test/shmemx/put_signal.c

test/shmemx/put_signal_nbi.c

test/shmemx/put_signal.c

Signed-off-by: Md <[email protected]>

davidozog · 2020-02-24T15:48:15Z

@wrrobin and @jdinan - I've just run through the release checklist with this branch included and had no problems. I haven't yet tested the GNI provider with this patch though, because Cori is under maintenance - but I don't expect to see any issues.

Are we ok with merging this and including in a v1.4.5 release today?

wrrobin · 2020-02-24T15:55:15Z

@davidozog Sounds good to me. I tested with GNI (Cori) last time and all the unit and shmemx tests passed.

wrrobin added 4 commits February 6, 2020 13:13

Signal operation type for put with signal

1b471e5

Signed-off-by: Md <[email protected]>

Added changes to portals4 put-signal implementation

be9e259

Signed-off-by: Md <[email protected]>

Correction for instance with len = 0

67912f5

Signed-off-by: Md <[email protected]>

Correction in API call for instance with len = 0

42a61cf

jdinan requested changes Feb 6, 2020

View reviewed changes

davidozog mentioned this pull request Feb 11, 2020

Update NEWS and MAJOR/MINOR_VERSION for v1.4.5 #930

Merged

wrrobin added 2 commits February 12, 2020 00:30

Trial hack for the fix related to on-node transport of signal atomic

217c430

Signed-off-by: Md <[email protected]>

Bug fix for missing function declaration and incorrect macro

bada86b

Signed-off-by: Md <[email protected]>

jdinan requested changes Feb 12, 2020

View reviewed changes

Update in configure script to enable shr atomics when only xpmem is e…

32bf17d

…nabled Signed-off-by: Md <[email protected]>

jdinan reviewed Feb 12, 2020

View reviewed changes

src/shmem_internal.h Show resolved Hide resolved

wrrobin added 2 commits February 12, 2020 15:32

Bug fix for data race in put_signal_nbi test

3af033e

Signed-off-by: Md <[email protected]>

Set the enable-shr-atomics to auto by default rather than disabled

e24e99a

Signed-off-by: Md <[email protected]>

wrrobin requested review from jdinan and davidozog February 13, 2020 15:39

jdinan approved these changes Feb 13, 2020

View reviewed changes

davidozog reviewed Feb 14, 2020

View reviewed changes

src/transport_portals4.h Outdated Show resolved Hide resolved

test/shmemx/put_signal.c Show resolved Hide resolved

test/shmemx/put_signal.c Outdated Show resolved Hide resolved

test/shmemx/put_signal_nbi.c Outdated Show resolved Hide resolved

test/shmemx/put_signal.c Show resolved Hide resolved

Moved signal op constants to shmemx.h; incorporated git comments

432835a

Signed-off-by: Md <[email protected]>

davidozog merged commit c784107 into Sandia-OpenSHMEM:master Feb 24, 2020

davidozog mentioned this pull request Feb 24, 2020

Inconsistent handling of unrecognized _Generic type (possibly) #933

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signal operation type for put with signal #929

Signal operation type for put with signal #929

wrrobin commented Feb 6, 2020

jdinan Feb 6, 2020

wrrobin Feb 7, 2020 •

edited

Loading

jdinan Feb 8, 2020

jdinan Feb 8, 2020

wrrobin Feb 11, 2020

jdinan Feb 11, 2020

wrrobin Feb 11, 2020

jdinan Feb 12, 2020

jdinan Feb 6, 2020

wrrobin commented Feb 12, 2020

jdinan Feb 12, 2020

jdinan Feb 12, 2020

wrrobin Feb 12, 2020

jdinan Feb 12, 2020

jdinan commented Feb 12, 2020

wrrobin commented Feb 12, 2020

jdinan Feb 12, 2020

jdinan left a comment

davidozog commented Feb 13, 2020

jdinan commented Feb 13, 2020

wrrobin commented Feb 14, 2020 •

edited

Loading

jdinan commented Feb 14, 2020

wrrobin commented Feb 14, 2020 •

edited

Loading

wrrobin commented Feb 14, 2020

davidozog left a comment

davidozog commented Feb 24, 2020

wrrobin commented Feb 24, 2020

Signal operation type for put with signal #929

Signal operation type for put with signal #929

Conversation

wrrobin commented Feb 6, 2020

Choose a reason for hiding this comment

wrrobin Feb 7, 2020 • edited Loading

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

wrrobin commented Feb 12, 2020

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

jdinan commented Feb 12, 2020

wrrobin commented Feb 12, 2020

Choose a reason for hiding this comment

jdinan left a comment

Choose a reason for hiding this comment

davidozog commented Feb 13, 2020

jdinan commented Feb 13, 2020

wrrobin commented Feb 14, 2020 • edited Loading

jdinan commented Feb 14, 2020

wrrobin commented Feb 14, 2020 • edited Loading

wrrobin commented Feb 14, 2020

davidozog left a comment

Choose a reason for hiding this comment

davidozog commented Feb 24, 2020

wrrobin commented Feb 24, 2020

wrrobin Feb 7, 2020 •

edited

Loading

wrrobin commented Feb 14, 2020 •

edited

Loading

wrrobin commented Feb 14, 2020 •

edited

Loading