Skip to content

UCP/FLUSH: Fix handling of error in flush if EP failed - #5976

Closed
dmitrygx wants to merge 1 commit into
openucx:masterfrom
dmitrygx:topic/ucp/flush_handle_err
Closed

dmitrygx wants to merge 1 commit into
openucx:masterfrom
dmitrygx:topic/ucp/flush_handle_err

Conversation

@dmitrygx

@dmitrygx dmitrygx commented Dec 1, 2020 •

Copy link
Copy Markdown
Member

What

Fix handling of error in flush if EP failed.

Why ?

If EP failed and UCT iface ops are replaced by the return UCS_ERR_ENDPOINT_TIMEOUT, it could fail flush in close operation.
It starts failing after removing FAILED flag check in flush - #5952
Fixes #5977

How ?

Check if EP failed and is being closed by the user, just return w/o reporting any errors.

Comment thread src/ucp/rma/flush.c Outdated
Comment on lines +27 to +28
UCP_EP_FLAG_FAILED |
UCP_EP_FLAG_CLOSED)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

aren't we already setting "level" based on error handling mode?
or the point of the PR is avoiding "req->status = status" so close_nb request will not fail?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

or the point of the PR is avoiding "req->status = status" so close_nb request will not fail?

yes, the point of this PR is to not set req->status to the error

aren't we already setting "level" based on error handling mode?

when it fails:

(gdb) p req->send.ep->worker->ep_config[req->send.ep->cfg_index]->key.err_mode
$2 = UCP_ERR_HANDLING_MODE_NONE

the peer failure wasn't requested by the test, but UCP unconditionally provides error callback to UCT

UCT_IFACE_PARAM_FIELD_ERR_HANDLER |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the peer failure wasn't requested by the test, but UCP unconditionally provides error callback to UCT

it's ok to provide callback to UCT. as long as we don't call user callback from UCP.
In this case, the test should suppress log message. It should not expect "silent" error, when ucp ep err mode is not set.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's ok to provide callback to UCT. as long as we don't call user callback from UCP.

yes, I know

In this case, the test should suppress log message. It should not expect "silent" error, when ucp ep err mode is not set.

I guess, we should fix all tests that do disconnect
but what should we do with close request status? ignore the status?

@dmitrygx dmitrygx Dec 1, 2020 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

also, we should not fail the close request if the UCP's ucp_worker_iface_error_handler() was called, but its scheduled on progress part (ucp_worker_iface_err_handle_progress()) was not completed yet.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think this is actually TCP issue. the test should not generate error at all.
it happens because sometimes the receiver does not get notification about remote connection close, so when it creates 2nd connection, it picks a "zombie" connection from conn_match

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it happens because sometimes the receiver does not get notification about remote connection close, so when it creates 2nd connection, it picks a "zombie" connection from conn_match

it may not get a remote connection close, but since we always increment connection match ID, when we create new connection, it will try to find remote peer with new ID

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we didn't increment it on receiver side so it stayed with 0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we didn't increment it on receiver side so it stayed with 0

do you mean that TCP EP lives for some time on a receiver side after its connection pair was disconnected, and then it fails and calls error handling callback?

@dmitrygx
dmitrygx force-pushed the topic/ucp/flush_handle_err branch from a0cf6f9 to 3050ee4 Compare December 2, 2020 06:48
@dmitrygx

dmitrygx commented Dec 2, 2020

Copy link
Copy Markdown
Member Author

bot:pipe:retest

@dmitrygx dmitrygx closed this Dec 2, 2020
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.

tcp/test_ucp_wireup_1sided.send_disconnect_reply2/3 failure

4 participants