UCX osc: make progress on idle worker if none are active #7632

devreal · 2020-04-15T10:41:18Z

When using UCX in shared memory progress is missing if one process waits in a barrier while the another process attempts to perform RMA operations. This PR adds progress on the first inactive workers if there are no active workers available.

Fixes #7631

Signed-off-by: Joseph Schuchart schuchart@hlrs.de

opal/mca/common/ucx/common_ucx_wpool.c

artpol84 · 2020-04-29T14:36:05Z

opal/mca/common/ucx/common_ucx_wpool.c

@@ -308,6 +309,10 @@ opal_common_ucx_wpool_progress(opal_common_ucx_wpool_t *wpool)
            }
            opal_mutex_unlock(&winfo->mutex);
        }
+        if (active_workers == 0 && opal_list_get_size(&wpool->idle_workers)) {


Actually, I think we need to progress it in any case.
It might happen that it's in the "idle" list even if we have other workers non-idling.

We are currently doing a significant refactoring of this code BTW.
But this is a good hint for us. So thank you very much.
For now, I think, we can just always progress this one. But with locking precautions.

The wpool->mutex is held at this point, that should be sufficient, right?

@devreal we need to progress the default worker.

devreal · 2020-06-29T15:07:39Z

@artpol84 Is there anything left to do here?

lanl-ompi · 2020-10-25T21:49:19Z

Can one of the admins verify this patch?

devreal · 2020-11-06T08:32:00Z

@artpol84 @janjust I rebased this PR onto current master. I was surprised to see that the wpool mutex is now take unconditionally (https://github.com/open-mpi/ompi/pull/7632/files#diff-4277b36854f67c21224bff1f16f6aa090af4e3905c9e00ce96dc735f05c90200R275). This seems like an unnecessary synchronization point, threads could do other things if another thread is already progressing, no?

artpol84 · 2020-11-11T02:41:17Z

opal/mca/common/ucx/common_ucx_wpool.c

+    if (active_workers == 0 && opal_list_get_size(&wpool->idle_workers)) {
+        /* make sure to progress at least some */
+        ucp_worker_progress(wpool->dflt_worker);
+    }


Suggested change

if (active_workers == 0 && opal_list_get_size(&wpool->idle_workers)) {

/* make sure to progress at least some */

ucp_worker_progress(wpool->dflt_worker);

}

ucp_worker_progress(wpool->dflt_worker);

artpol84 · 2020-11-11T02:43:47Z

opal/mca/common/ucx/common_ucx_wpool.c

@@ -282,6 +283,10 @@ opal_common_ucx_wpool_progress(opal_common_ucx_wpool_t *wpool)
        } while (progressed);


We can skip the default worker here to avoid double-progress.

But we can't skip progressing the default worker if there are active workers. The default worker might still not be active, but you want to progress it.

artpol84 · 2020-11-11T02:48:53Z

@devreal lets sync if you still would like to discuss this.
I'm planning to merge this ASAP.

devreal · 2020-11-11T21:43:41Z

@artpol84 I made a couple of changes:

Store the dflt_winfo in the wpool instead of the dflt_worker to allow for locking it in case the default worker has to be progressed.
Avoid blocking on a taken mutex (which actually lead to deadlocks otherwise, presumably due to lock inversion).
Avoid progressing the dflt_worker directly in osc/ucx but instead call opal_common_ucx_wpool_progress due to ensure proper locking.
I found that the winfo objects are allocated using calloc but then released using OBJ_RELEASE. I don't see how that ever can have worked... I tired to fix it, please review carefully.

bosilca · 2020-11-11T22:06:06Z

Calloc/Malloc + OBJ_CONSTRUCT is morally equivalent to OBJ_NEW. It can therefore be used with OBJ_RELEASE.

devreal · 2020-11-11T22:08:01Z

@bosilca I am aware of that, but there was no OBJ_CONSTRUCT as far as I can see. Maybe I am just blind here...?

Signed-off-by: Joseph Schuchart <schuchart@hlrs.de>

artpol84 · 2020-11-13T06:35:47Z

@devreal as I mentioned we have a PR ready with a lot of cleanups and stability fixes. Including more active use of opal objects.
@janjust please see if you’ll be able to rebase on top of this one.
If not- maybe you can port these changes instead keeping the original authorship of @devreal

artpol84

Overall looks ok.

Though please let @janjust to check what’s the easier sequence of commits in the context of upcoming refactoring.

artpol84 · 2020-11-13T06:40:24Z

opal/mca/common/ucx/common_ucx_wpool.c

@@ -272,17 +278,33 @@ opal_common_ucx_wpool_progress(opal_common_ucx_wpool_t *wpool)
    /* Go over all active workers and progress them
     * TODO: may want to have some partitioning to progress only part of
     * workers */
-    opal_mutex_lock(&wpool->mutex);
+    if (0 != opal_mutex_trylock(&wpool->mutex)) {


We have this in @janjust pr already.
Maybe we should have merged it first :(

janjust · 2020-11-13T20:56:15Z

@janjust please see if you’ll be able to rebase on top of this one.
If not- maybe you can port these changes instead keeping the original authorship of @devreal

@devreal @artpol84 rebasing is fine, let's get it merged

artpol84 · 2020-11-13T21:27:40Z

Thanks a lot!

devreal requested review from yosefe and artpol84 April 15, 2020 10:41

artpol84 reviewed Apr 22, 2020

View reviewed changes

opal/mca/common/ucx/common_ucx_wpool.c Outdated Show resolved Hide resolved

gpaulsen added Target: main Target: v5.0.x labels Apr 24, 2020

devreal force-pushed the osc-ucx-progress branch from f22c532 to 9c03904 Compare April 29, 2020 07:26

artpol84 reviewed Apr 29, 2020

View reviewed changes

devreal force-pushed the osc-ucx-progress branch from 9c03904 to 64856d2 Compare November 6, 2020 08:29

artpol84 requested changes Nov 11, 2020

View reviewed changes

devreal force-pushed the osc-ucx-progress branch from 64856d2 to f3e632a Compare November 11, 2020 21:37

UCX osc: make progress on default worker if none are active

581478d

Signed-off-by: Joseph Schuchart <schuchart@hlrs.de>

devreal force-pushed the osc-ucx-progress branch from f3e632a to 581478d Compare November 11, 2020 22:23

artpol84 approved these changes Nov 13, 2020

View reviewed changes

artpol84 merged commit f9ef4b4 into open-mpi:master Nov 13, 2020

devreal mentioned this pull request Nov 16, 2020

osc/ucx: Make wpctx global #8212

Closed

This was referenced Mar 11, 2021

UCX hangs in MPI_Win_lock on master #6816

Closed

SHMEM: missing progress when target is polling locally #6924

Closed

devreal deleted the osc-ucx-progress branch October 3, 2022 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCX osc: make progress on idle worker if none are active #7632

UCX osc: make progress on idle worker if none are active #7632

devreal commented Apr 15, 2020

artpol84 Apr 29, 2020

artpol84 Apr 29, 2020

devreal Apr 29, 2020

artpol84 Nov 11, 2020

devreal commented Jun 29, 2020

lanl-ompi commented Oct 25, 2020

devreal commented Nov 6, 2020

artpol84 Nov 11, 2020

artpol84 Nov 11, 2020

artpol84 commented Nov 11, 2020

devreal commented Nov 11, 2020

bosilca commented Nov 11, 2020 •

edited

Loading

devreal commented Nov 11, 2020

artpol84 commented Nov 13, 2020

artpol84 left a comment

artpol84 Nov 13, 2020

janjust commented Nov 13, 2020

artpol84 commented Nov 13, 2020

		@@ -282,6 +283,10 @@ opal_common_ucx_wpool_progress(opal_common_ucx_wpool_t *wpool)
		} while (progressed);

UCX osc: make progress on idle worker if none are active #7632

UCX osc: make progress on idle worker if none are active #7632

Conversation

devreal commented Apr 15, 2020

artpol84 Apr 29, 2020

Choose a reason for hiding this comment

artpol84 Apr 29, 2020

Choose a reason for hiding this comment

devreal Apr 29, 2020

Choose a reason for hiding this comment

artpol84 Nov 11, 2020

Choose a reason for hiding this comment

devreal commented Jun 29, 2020

lanl-ompi commented Oct 25, 2020

devreal commented Nov 6, 2020

artpol84 Nov 11, 2020

Choose a reason for hiding this comment

artpol84 Nov 11, 2020

Choose a reason for hiding this comment

artpol84 commented Nov 11, 2020

devreal commented Nov 11, 2020

bosilca commented Nov 11, 2020 • edited Loading

devreal commented Nov 11, 2020

artpol84 commented Nov 13, 2020

artpol84 left a comment

Choose a reason for hiding this comment

artpol84 Nov 13, 2020

Choose a reason for hiding this comment

janjust commented Nov 13, 2020

artpol84 commented Nov 13, 2020

bosilca commented Nov 11, 2020 •

edited

Loading