Oshmem multiple contexts #6492

janjust · 2019-03-15T17:03:04Z

The PR addresses several issues with creating multiple contexts in oshmem/ucx.

This reverts commit f1b095c. Signed-off-by: Tomislav Janjusic <[email protected]>

Signed-off-by: Tomislav Janjusic <[email protected]>

Signed-off-by: Xin Zhao <[email protected]> Signed-off-by: Tomislav Janjusic <[email protected]>

Signed-off-by: Tomislav Janjusic <[email protected]>

… track of ucx_ctx_default's rkeys Signed-off-by: Tomislav Janjusic <[email protected]>

ompiteam-bot · 2019-03-15T17:03:08Z

Can one of the admins verify this patch?

janjust · 2019-03-15T17:04:17Z

@yosefe @brminich @hoopoepg @xinzhao3 @jladd-mlnx
The PR is up, as changes are requested we'll push against this PR.

jsquyres · 2019-03-15T17:39:49Z

ok to test

ibm-ompi · 2019-03-15T17:50:11Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

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

ibm-ompi · 2019-03-15T17:50:27Z

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

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

jsquyres · 2019-03-15T17:52:13Z

@janjust Looks like there's some compile errors in the UCX stuff. Can you update/fix?

janjust · 2019-03-15T18:12:09Z

@jsquyres fixed, last minute squash went wrong :(

artpol84 · 2019-03-15T20:13:04Z

@jsquyres @bwbarrett build checker failed with obviously unrelated error:

FATAL: Channel "unknown": Remote call on JNLP4-connect connection from 
proxy-s1.lanl.gov/192.12.184.6:36160 failed. The channel is closing down or has closed down

Who is the right person to ask for investigation.

jsquyres · 2019-03-15T20:17:24Z

Just re-run the test and see if the same thing happens.

bot:ompi:retest

hoopoepg · 2019-03-16T05:30:34Z

oshmem/mca/spml/ucx/spml_ucx.c

+
+    if (array->ctxs_count < array->ctxs_num) {
+        array->ctxs[array->ctxs_count] = ctx;
+    }


minor: } else {

hoopoepg · 2019-03-16T05:47:02Z

oshmem/mca/spml/ucx/spml_ucx.h

-
 typedef spml_ucx_mkey_t * (*mca_spml_ucx_get_mkey_slow_fn_t)(shmem_ctx_t ctx, int pe, void *va, void **rva);

+typedef struct mca_spml_ucx_ctx_array {


what about use linked list instead of array? it is much simpler to operate with list entries (add/remove/shift between lists) than array elements, there are set of macro in opal to manipulate linked lists

So you weren't part of the discussion leading up to this PR, but @yosefe suggested to use a similar implementation to opal callback, hence why the arrays - it's possibly faster, I haven't measured.

hoopoepg · 2019-03-16T05:56:56Z

opal/mca/common/ucx/common_ucx.c

-OPAL_DECLSPEC int opal_common_ucx_del_procs(opal_common_ucx_del_proc_t *procs, size_t count,
-                                            size_t my_rank, size_t max_disconnect, ucp_worker_h worker)
-{
+OPAL_DECLSPEC int opal_common_ucx_del_procs_nb(opal_common_ucx_del_proc_t *procs, size_t count,


minor: this call is blocking on wait all disconnection requests... _nb may confuse :)

you're right, so the difference between the two is that the original had a fence which in oshmem case is a global sync, I'll remove _nb, maybe add _nofence to the call.

hoopoepg · 2019-03-16T06:05:16Z

oshmem/mca/spml/ucx/spml_ucx.c

+    }
+    else {
+        array->ctxs = realloc(array->ctxs, (array->ctxs_num + 8) * sizeof(mca_spml_ucx_ctx_t *));
+        opal_atomic_wmb ();


why we need write barrier here? what data should be stored?

For the same reason why it's done in _opal_progress_register(); opal_progress.c:409, although it doesn't use a realloc, but it looks like it reimplements realloc - not sure why. So to keep consistent I added a wmb() after the realloc().

hoopoepg · 2019-03-16T06:25:12Z

oshmem/mca/spml/ucx/spml_ucx.c


    SHMEM_MUTEX_LOCK(mca_spml_ucx.internal_mutex);
+    _ctx_remove(&mca_spml_ucx.active_array, (mca_spml_ucx_ctx_t *)ctx);
+    _ctx_add(&mca_spml_ucx.idle_array, (mca_spml_ucx_ctx_t *)ctx);


you are removing context from progress, all outstanding operations should be flushed

as I can see context is not destroyed here - it is just moved into idle array and all outstanding ops may be left uncompleted. ucp_worker_flush may depend from remote side for fetch_and_op calls, but as I can see user can't remove OSHMEM context when there are incomplete ops, like called ***_nbi and after this call ctx_destroy without call shmem_ctx_quiet... but I'm not sure about this :)

it should be ok, because disconnect_nb (invoked by ctx_cleanup) will flush eps

from the other side, flushing worker may help to avoid 'hard-to-debug' issues. I tend to agree with @hoopoepg that worker flush is worth to be done here. @yosefe, wdyt?

added flush() - @xinzhao3 need to run this through unit tests again

brminich · 2019-03-17T07:15:47Z

opal/mca/common/ucx/common_ucx.c

+
+    if (OPAL_SUCCESS != (ret = opal_pmix.fence_nb(NULL, 0,
+                    opal_common_ucx_mca_fence_complete_cb, (void*)fenced))){
+        return ret;


no need to check ret here. Can just return opal_pmix.fence_nb (or just use it without this wrapper)

brminich · 2019-03-17T07:20:25Z

opal/mca/common/ucx/common_ucx.c

+{
+    int ret = OPAL_SUCCESS;
+    opal_common_ucx_del_procs_nb(procs, count, my_rank, max_disconnect, worker);
    if (OPAL_SUCCESS != (ret = opal_common_ucx_mca_pmix_fence(worker))) {


no need to check ret, can just return

brminich · 2019-03-17T08:06:48Z

oshmem/mca/spml/ucx/spml_ucx_component.c

+    mca_spml_ucx.active_array.ctxs = calloc(mca_spml_ucx.active_array.ctxs_num,
+                                            sizeof(mca_spml_ucx_ctx_t *));
+    mca_spml_ucx.idle_array.ctxs = calloc(mca_spml_ucx.idle_array.ctxs_num,
+                                            sizeof(mca_spml_ucx_ctx_t *));


minor: indentation

brminich · 2019-03-17T08:18:27Z

oshmem/mca/spml/ucx/spml_ucx.c

+        array->ctxs[array->ctxs_count] = ctx;
+    }
+    else {
+        array->ctxs = realloc(array->ctxs, (array->ctxs_num + 8) * sizeof(mca_spml_ucx_ctx_t *));


better use macro definition for 8 instead of number

done in latest commit

ibm-ompi · 2019-03-18T17:51:23Z

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/7f03afbb3d3d229153a57dd21dc90058

ibm-ompi · 2019-03-18T17:53:27Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

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

brminich · 2019-03-19T06:12:32Z

oshmem/mca/spml/ucx/spml_ucx.c

    _ctx_add(&mca_spml_ucx.idle_array, (mca_spml_ucx_ctx_t *)ctx);
    SHMEM_MUTEX_UNLOCK(mca_spml_ucx.internal_mutex);

+    ucp_worker_flush(ucx_ctx->ucp_worker);


I did not notice that quiet(ctx) is called in the beginning of this function, so it makes no sense to call flush here then.
I'm sorry for this confusion.

yep, my fault. in current implementation of quiet there is flush_nb is called and it has own progress loop for worker (independent from opal progress).

ok I'll remove it

done, removed flush

jladd-mlnx · 2019-03-21T16:16:42Z

👍 looks good, team.

jsquyres

Is e294277 fixes to prior commits on this PR?

If so, please fix the problem in the respective commits themselves -- do not make commits and then have additional commits to fix the original commits.

Thanks.

Signed-off-by: Tomislav Janjusic <[email protected]>

…s/delete oshmem_barrier in shmem_ctx_destroy ompi/oshmem/spml/ucx: optimize spml ucx progress Signed-off-by: Tomislav Janjusic <[email protected]>

janjust · 2019-03-21T21:10:43Z

@jsquyres done, remove commit, and addressed comments in respective commits.

jsquyres · 2019-03-21T21:20:07Z

@janjust Many thanks. You'll probably need to update the corresponding cherry-pick hash notices in #6509, too.

xinzhao3 added 5 commits March 15, 2019 18:46

Revert "OMPI/OSHMEM: bug-fix: store mkeys for each oshmem ctx."

b00209e

This reverts commit f1b095c. Signed-off-by: Tomislav Janjusic <[email protected]>

ompi/oshmem/spml/ucx: fix eps destroy in shmem_ctx_destroy().

79ba752

Signed-off-by: Tomislav Janjusic <[email protected]>

OMPI/OSHMEM: bug-fix: store mkeys for each oshmem ctx.

289595e

Signed-off-by: Xin Zhao <[email protected]> Signed-off-by: Tomislav Janjusic <[email protected]>

ompi/oshmem/spml/ucx: let shmem_finalize to clean up any ctx left

9a06000

Signed-off-by: Tomislav Janjusic <[email protected]>

ompi/oshmem: add spml_context back to sshmem_type in memheap, to keep…

48033ac

… track of ucx_ctx_default's rkeys Signed-off-by: Tomislav Janjusic <[email protected]>

janjust force-pushed the oshmem-multiple-contexts-master branch from c8e5405 to 280f330 Compare March 15, 2019 18:11

hoopoepg reviewed Mar 16, 2019

View reviewed changes

brminich reviewed Mar 17, 2019

View reviewed changes

janjust force-pushed the oshmem-multiple-contexts-master branch from 6edc838 to db7541e Compare March 18, 2019 17:34

janjust force-pushed the oshmem-multiple-contexts-master branch from db7541e to b6d07da Compare March 18, 2019 18:02

brminich approved these changes Mar 19, 2019

View reviewed changes

janjust force-pushed the oshmem-multiple-contexts-master branch from b6d07da to e294277 Compare March 19, 2019 13:49

brminich approved these changes Mar 19, 2019

View reviewed changes

hoopoepg approved these changes Mar 19, 2019

View reviewed changes

janjust mentioned this pull request Mar 20, 2019

v4.0.x: Oshmem multiple contexts #6509

Merged

hoopoepg approved these changes Mar 21, 2019

View reviewed changes

jsquyres reviewed Mar 21, 2019

View reviewed changes

xinzhao3 added 3 commits March 21, 2019 23:01

ompi/oshmem/spml/ucx: defer clean up shmem_ctx to shmem_finalize

e1c1ab0

Signed-off-by: Tomislav Janjusic <[email protected]>

ompi/oshmem/spml/ucx:delete oob path of getting rkeys in spml ucx

e041400

Signed-off-by: Tomislav Janjusic <[email protected]>

ompi/oshmem/spml/ucx: use lockfree array to optimize spml_ucx_progres…

9c3d00b

…s/delete oshmem_barrier in shmem_ctx_destroy ompi/oshmem/spml/ucx: optimize spml ucx progress Signed-off-by: Tomislav Janjusic <[email protected]>

janjust force-pushed the oshmem-multiple-contexts-master branch from e294277 to 9c3d00b Compare March 21, 2019 21:06

jladd-mlnx merged commit 9ab6ecb into open-mpi:master Mar 22, 2019

janjust deleted the oshmem-multiple-contexts-master branch March 27, 2025 02:03


		typedef spml_ucx_mkey_t * (mca_spml_ucx_get_mkey_slow_fn_t)(shmem_ctx_t ctx, int pe, void va, void **rva);

		typedef struct mca_spml_ucx_ctx_array {

Oshmem multiple contexts #6492

Oshmem multiple contexts #6492

Uh oh!

Conversation

janjust commented Mar 15, 2019

Uh oh!

ompiteam-bot commented Mar 15, 2019

Uh oh!

janjust commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsquyres commented Mar 15, 2019

Uh oh!

ibm-ompi commented Mar 15, 2019

Uh oh!

ibm-ompi commented Mar 15, 2019

Uh oh!

jsquyres commented Mar 15, 2019

Uh oh!

janjust commented Mar 15, 2019

Uh oh!

artpol84 commented Mar 15, 2019

Uh oh!

jsquyres commented Mar 15, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibm-ompi commented Mar 18, 2019

Uh oh!

ibm-ompi commented Mar 18, 2019

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!

jladd-mlnx commented Mar 21, 2019

Uh oh!

jsquyres left a comment

janjust commented Mar 15, 2019 •

edited

Loading