Skip to content

btl/uct: correctly set the completion status before using uct complet… #13032

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

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Jan 10, 2025

…ions

According to UCT documentation the status field must be preset to UCS_OK before the completion is used. My assumption here is that the field is not filled in in all cases leaving the value as potentially garbage. This CL addresses the issue but setting the status in the constructor for both RDMA completions and frags. The status is then reset on completion callback.

@hjelmn hjelmn requested a review from yosefe January 10, 2025 05:48
@hjelmn hjelmn force-pushed the fix_flaw_in_btl_uct_because_i_did_not_fully_read_the_documentation_when_the_callbacks_were_changes_and_noone_noticed branch from 3e289b1 to f3baeba Compare January 10, 2025 05:49
…ions

According to UCT documentation the status field must be preset to UCS_OK before
the completion is used. My assumption here is that the field is not filled in in
all cases leaving the value as potentially garbage. This CL addresses the issue
but setting the status in the constructor for both RDMA completions and frags.
The status is then reset on completion callback.

Signed-off-by: Nathan Hjelm <hjelmn@google.com>
@jsquyres jsquyres force-pushed the fix_flaw_in_btl_uct_because_i_did_not_fully_read_the_documentation_when_the_callbacks_were_changes_and_noone_noticed branch from f3baeba to d586292 Compare January 10, 2025 19:05
@jsquyres
Copy link
Member

I rebased this PR (no content change) in an attempt to try to clear the RTD CI error.

@jsquyres
Copy link
Member

RTD CI problem resolved. @hjelmn George approved, so if you're good with this, go ahead and merge.

@hjelmn hjelmn merged commit 709c74c into open-mpi:main Jan 13, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants