OSC/UCX: Adding locks to win attach/detach and fixing build warnings #10593

MamziB · 2022-07-21T16:47:17Z

OSC/UCX: Adding locks to win attach/detach and fixing build warnings

Signed-off-by: Mamzi Bayatpour mbayatpour@nvidia.com

devreal · 2022-07-21T17:07:42Z

ompi/mca/osc/ucx/osc_ucx_component.c

    int insert_index = -1, contain_index;
    int ret = OMPI_SUCCESS;

+    ompi_osc_ucx_lock(MPI_LOCK_EXCLUSIVE,


Shouldn't we take a shared lock in get_dynamic_win_info to make sure readers get consistent segments?

@devreal I think the process that is calling get_dynmic_win_info should have already had the lock on target. That's why I did not want to lock it again.

You're right. But this won't work: the process that attaches the segment can hold a shared lock on the window, so acquiring an exclusive lock will deadlock. The osc/rdma component has two different locks for that reason

@devreal Alright, I'll make the necessary changes. Thanks for the feedback.

@devreal can you let me know your thoughts about the second commit?

Yeah, I think that looks good. One last thing: it's now not just the atomicity lock anymore. So should we rename it to state lock? That might avoid some confusion for future readers...

Thanks for the feedback, I really appreciate it. Yes, I just modified it. Please take a look at the single commit.

devreal · 2022-07-25T17:01:53Z

ompi/mca/osc/ucx/osc_ucx_comm.c

    int insert = -1;
    int ret;

+    bool lock_needed = ompi_osc_need_acc_lock(module, ompi_comm_rank(module->comm));


Shouldn't that be ompi_osc_need_acc_lock(module, target);? And same below for the lock call.

Oh yes, good catch. It should be locking the target window. I will make the change.

devreal · 2022-07-25T17:09:10Z

ompi/mca/osc/ucx/osc_ucx_comm.c


+    bool lock_needed = ompi_osc_need_acc_lock(module, ompi_comm_rank(module->comm));
+    if (lock_needed) {
+        ompi_osc_ucx_lock(MPI_LOCK_SHARED, ompi_comm_rank(module->comm), 0, win);


Is this the right lock? It seems like this should be the accumulate lock, like in start_atomicity?

Since inside the win attach, we are locking OSC_UCX_STATE_LOCK_OFFSET offset, I think we need to try to get the same lock from the process that is calling get_dynamic_info. Thoughts?

I was just wondering: does ompi_osc_ucx_lock take the same lock as MPI_Win_lock? The name sounds like it... In that case we'd still run into a deadlock if the calling process already owns an MPI-level locks on the window.

yes, when I fixed the typo of the value of the target lock on get_dynamic_info, now I get an error complaining that I am trying to get a lock that was already got acquired (MPI_ERR_RMA_SYNC). It seems like MPI_Win_lock ends up locking the same lock as ompi_osc_ucx_lock. In this case, we need to use ACC lock for both window-attach and get_dynamic_info.

@devreal I made the above changes. Please take a look.

ompi/mca/osc/ucx/osc_ucx_component.c

devreal

Getting there :)

devreal · 2022-07-25T21:47:22Z

ompi/mca/osc/ucx/osc_ucx_comm.c

-}
-
-static inline int get_dynamic_win_info(uint64_t remote_addr, ompi_osc_ucx_module_t *module,
+static inline int get_dynamic_win_info(uint64_t remote_addr, struct ompi_win_t *win,


We don't seem to use win anymore, maybe revert back to module here and in CHECK_DYNAMIC_WIN?

Sure, sounds good. Let me add a new commit.

devreal · 2022-07-25T21:50:26Z

ompi/mca/osc/ucx/osc_ucx_comm.c

    }

-    CHECK_DYNAMIC_WIN(remote_addr, module, target, ret);
+    CHECK_DYNAMIC_WIN(remote_addr, module, target, ret, win);


I think there is still an issue: if !module->acc_single_intrinsic we take the state lock and then would take the state lock again in get_dynamic_win_info. Maybe check in get_dynamic_win_info if the state lock was taken (by passing lock_acquired from above)?

Good catch. Yes, we need to check that. I'll add this.

devreal

One minor comment. Please squash and then it's ready to go 👍

devreal · 2022-07-26T01:30:11Z

ompi/mca/osc/ucx/osc_ucx_component.c

                                                      base, len, insert);
    }
 }
+extern bool ompi_osc_need_acc_lock(ompi_osc_ucx_module_t *module, int target)


No need for extern here and on the functions below since it has been declared extern inline already

Not ready. @devreal Actually, looks like the new change (using ACC lock, instead of the main application lock) leads to hang at a large scale. The reason is that we must not perform window attach, while other processes are accessing this dynamic window. Currently, ACC lock only remains alive during the get_dynamic_window_info() which is not enough. We must lock ACC-lock during entire RMA operation. Working on this change.

I'm just thinking out loud. Since we only need lock to read the window info (not the data inside the window), Ideally, we should not need to have the acc-lock during entire rma operation, but somehow, using this extra acc lock instead of app lock is leading to race condition.

The reason is that we must not perform window attach, while other processes are accessing this dynamic window.

I'm not sure I understand the issue. Why not? We shouldn't keep the acc longer than we need to, i.e., we should release it as soon as we have queried the attached segments.

I need to investigate further to know exactly what's wrong with the current state of the patch that leads to hang. I don't get the hang if I omit the today's changes (adding a c-lock). I will update you.

hey, @devreal I fixed the hang issue, please let me know if you have any more comments. Thanks.

devreal · 2022-07-26T18:58:38Z

Looks good to me now, please squash and I will approve

MamziB · 2022-07-26T20:55:11Z

@devreal Thanks for the feedback. The commit is ready.

Signed-off-by: Mamzi Bayatpour <mbayatpour@nvidia.com>

awlauria · 2022-08-02T16:37:55Z

@MamziB please cherry-pick to v5.0.x. Thanks

github-actions bot added the Target: main label Jul 21, 2022

devreal reviewed Jul 21, 2022

View reviewed changes

devreal reviewed Jul 25, 2022

View reviewed changes

MamziB force-pushed the mamzi/dynamic-win-fixes-2 branch from 2f0ec89 to d9becec Compare July 25, 2022 21:44

devreal reviewed Jul 25, 2022

View reviewed changes

devreal reviewed Jul 26, 2022

View reviewed changes

MamziB force-pushed the mamzi/dynamic-win-fixes-2 branch 2 times, most recently from 6496081 to 723f17a Compare July 26, 2022 20:54

OSC/UCX: Adding locks to win attach/deattach and fixing build warnings

56afecd

Signed-off-by: Mamzi Bayatpour <mbayatpour@nvidia.com>

MamziB force-pushed the mamzi/dynamic-win-fixes-2 branch from 723f17a to 56afecd Compare July 26, 2022 20:55

devreal approved these changes Jul 26, 2022

View reviewed changes

awlauria merged commit 5c302ac into open-mpi:main Aug 2, 2022

OSC/UCX: Adding locks to win attach/detach and fixing build warnings #10593

OSC/UCX: Adding locks to win attach/detach and fixing build warnings #10593

Uh oh!

Conversation

MamziB commented Jul 21, 2022

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!

MamziB Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devreal left a comment

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!

devreal left a comment

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!

devreal commented Jul 26, 2022

Uh oh!

MamziB commented Jul 26, 2022

Uh oh!

awlauria commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MamziB Jul 25, 2022 •

edited

Loading