Repository navigation
Fixes for iodemo and listener destroy flow - #5982
Conversation
If a listener is destroyed while it has a pending connect request which has not been dispatched yet, we must release it. Otherwise, rdma_destroy_id() will hang while waiting for all events to be released.
|
@evgeny-leksikov @alinask pls take a look |
| ucs_trace("listener %p: destroying", listener); | ||
|
|
||
| UCS_ASYNC_BLOCK(&listener->worker->async); | ||
| ucs_callbackq_remove_if(&listener->worker->uct->progress_q, |
There was a problem hiding this comment.
i must be missing something here, where is this function added to the queue?
ucp_cm_server_conn_request_progress_cb_pred()
There was a problem hiding this comment.
AFAIU ucp_cm_server_conn_request_progress_cb_pred is a predicate callback to remove ucp_cm_server_conn_request_progress from the queue and reject conn request
There was a problem hiding this comment.
@alinask it's added in ucp_cm_server_conn_request_cb but not removed in ucp_listener_destroy
There was a problem hiding this comment.
@yosefe can you please point to where it's added in ucp_cm_server_conn_request_cb?
There was a problem hiding this comment.
ucx/src/ucp/wireup/wireup_cm.c
Line 952 in c786a06
There was a problem hiding this comment.
thanks but meant where ucp_cm_server_conn_request_progress_cb_pred() is added.
it's kind of an 'extension' of ucp_cm_server_conn_request_progress?
There was a problem hiding this comment.
ucp_cm_server_conn_request_progress_cb_pred is not added, it's just helper function for ucs_callbackq_remove_if
| delete iter->second; | ||
| UCX_LOG << "destroy_connections"; | ||
| while (!_conns.empty()) { | ||
| delete _conns.begin()->second; |
There was a problem hiding this comment.
seems like "double free", this way clean up needs pop_front/erase here and _conns.clear() below is redundant
this can re-balance the map on erase, so old code is better
There was a problem hiding this comment.
removed clear()
agree that it can cause redundant rebalance, but IMO it's simpler (than to move all connections to tmp list), and this code part is exit cleanup so perf (up to logn) should not be important
There was a problem hiding this comment.
ok on rebalance impact but I still don't understand how does it work:
delete _conns.begin()->second; deletes the second object but doesn't erase begin() element from the map, so while (!_conns.empty()) { is infinite loop which should cause multiple destruction/free of the same object
There was a problem hiding this comment.
the destructor of UcxConnection removes itself from the map
i'll add comment
There was a problem hiding this comment.
👍 so that means the old code was actually incorrect
| ucs_trace("listener %p: destroying", listener); | ||
|
|
||
| UCS_ASYNC_BLOCK(&listener->worker->async); | ||
| ucs_callbackq_remove_if(&listener->worker->uct->progress_q, |
There was a problem hiding this comment.
AFAIU ucp_cm_server_conn_request_progress_cb_pred is a predicate callback to remove ucp_cm_server_conn_request_progress from the queue and reject conn request
3d5c076 to
ca8ed8d
Compare
- Destroy all connections to flush requests before destroying memory pools - Avoid removing elements from connections map while iterating over it
ca8ed8d to
3fd1a31
Compare
Why
Fix IODEMO server exit and destructors flow