-
Notifications
You must be signed in to change notification settings - Fork 882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UCX osc: properly release exclusive lock to avoid lockup #6933
Conversation
int ret = OMPI_SUCCESS; | ||
|
||
ret = opal_common_ucx_wpmem_fetch(module->state_mem, | ||
UCP_ATOMIC_FETCH_OP_SWAP, TARGET_LOCK_UNLOCKED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please elaborate a bit about the hang? If we set TARGET_LOCK_UNLOCKED
(which is zero), start_shared
should succeed, because result_value
is supposed to be less than TARGET_LOCK_EXCLUSIVE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume that process 0 holds an exclusive lock and process 1 tries to acquire a shared lock. Process 1 will retrieve the lock value (which is TARGET_LOCK_EXCLUSIVE
) and add 1
it, making it TARGET_LOCK_EXCLUSIVE + 1
. In the meantime, however, process 0 releases the lock by resetting it to TARGET_LOCK_UNLOCKED
(which is 0). Since Process 1 has seen the lock as being taken exclusively, it will subtract 1
again, leading to a value of -1
in the lock variable (the lock is thus out of sync).
The next time Process 1 tries to take a shared lock, it will get the value -1 in a variable of type uint64_t
, which is definitely >=TARGET_LOCK_EXCLUSIVE
.
I realized, though, the my patch used -((int64_t)TARGET_LOCK_UNLOCKED)
instead of -((int64_t)TARGET_LOCK_EXCLUSIVE)
. That should be fixed now. Sorry if that led to confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now, thanks
Signed-off-by: Joseph Schuchart <schuchart@hlrs.de>
08cb638
to
a5cc380
Compare
I notice this was cherry-picked to the 3.1.x branch. Do we want this for the 4.0 branches? Edit: I see it was merged to 4.0.x a while ago. |
4.0.x: #6934 |
Fixes #6931
Signed-off-by: Joseph Schuchart schuchart@hlrs.de