Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UCP/CORE: Implement flush+destroy for UCT EPs on UCP Worker #5608
UCP/CORE: Implement flush+destroy for UCT EPs on UCP Worker #5608
Changes from 26 commits
f7b0e3c
627a8a5
e9d3b1d
ecd8663
8d4a9f9
d229eae
8558d4c
cdfe88d
c1a8fee
28edf67
022a6f2
46b1272
cb197a4
5d0b3fa
12d1c34
d1ab7d4
d809dc1
7e6a5b4
a0f759b
46afaaf
e2982d5
7ecd09d
9ef1f34
efdb4d3
509dd6c
07b8bb3
43c3e92
9153e50
9c346d6
465be85
cdb8875
2a5888f
d3cf051
ea18b51
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
UCS_OK is not handled?
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.
it is handled in the
else
sectionThere 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.
this assert looks wrong since status value is returned by
flush
hereThere 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.
this is not wrong, just to make sure that we will not enter the endless loop if
uct_ep_flush()
returnsUCS_ERR_BUSY
added the comment
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.
but uct_ep_flush doc does not say that it cannot return
UCS_ERR_BUSY
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.
yes, will handle it
@yosefe what do you think to use progress_register() instead of this loop?
seems to be error-prone or overcomplicated if it will have several
retrun
/break
/continue
in the loopThere 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.
if status != NO_RESOURCE should also return UCS_OK
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.
if flush returned ok, who will call the completion?
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.
why? we should put to the pending and wait for the callback invocation
it will be done in
ucp_worker_discard_uct_ep_flush_comp()
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.
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.
ah, I see. fixed
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.
seems weird recursion: ucp_worker_discard_uct_ep->ucp_worker_discard_wireup_ep->ucp_worker_discard_uct_ep
maybe separate the 2nd part of this function to a helper:
if wireup:
discard_wireup_ep()
if next_ep_owner:
discard_uct_ep_helper(next_ep) [works only on regular uct_ep, not wireup]
else:
discard_uct_ep_helper(uct_ep)
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.
but what the problem with this recursion?
ucp_worker_discard_uct_ep->ucp_worker_discard_wireup_ep
for WIREUP EPand
->ucp_worker_discard_uct_ep
for AUX EPThere 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.
imo would be better /wo recursion, since we know next_ep cannot be wireup_ep
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.
done
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.
minor: maybe just
kh_put(ucp_worker_discard_uct_ep_hash,...
? Then no need in unusediter
varThere 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.
done