Skip to content

osc/ucx: flush all the outstanding transfers before cleaning up the thread local context #10238

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

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

MamziB
Copy link
Contributor

@MamziB MamziB commented Apr 7, 2022

No description provided.

…hread local context

Signed-off-by: Mamzi Bayatpour  <[email protected]>
Co-authored-by: Tomislav Janjusic <[email protected]>
@janjust janjust requested review from janjust and awlauria April 7, 2022 18:40
@awlauria
Copy link
Contributor

awlauria commented Apr 7, 2022

bot:aws:retest

winfo->global_inflight_ops = 0;
memset(winfo->inflight_ops, 0, winfo->comm_size * sizeof(short));
opal_mutex_unlock(&winfo->mutex);
if (rc != OPAL_SUCCESS) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be better to move the check for failure inside of the mutex (but of course remember to unloc before returning).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rc is a local variable, so it is immune to being changed by another thread for what it is worth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I was going to ask - why move it inside the mutex?

Copy link
Member

@gpaulsen gpaulsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too passionate about moving the check for error inside the mutex.

@janjust janjust merged commit f377569 into open-mpi:main Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants