RC and iodemo fixes #71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Owner
yosefe
commented
Oct 27, 2020
•
yosefe
commented
- Reserve 1 CQ credit for qp-flush NOP, to avoid disabling CQ moderation
- Fix keepalive on DevX
- Fix memory leak in IO demo
- Simplify active connection counting
|
@evgeny-leksikov pls take a look |
test/apps/iodemo/io_demo.cc
Outdated
| @@ -671,9 +673,14 @@ class DemoServer : public P2pDemoCommon { | |||
| recv_data(conn, *iov, msg->tr.sn, w); | |||
| } | |||
|
|
|||
| virtual void dispatch_connection_accepted(UcxConnection* conn) {\ | |||
test/apps/iodemo/ucx_wrapper.h
Outdated
Comment on lines
140
to
142
| bool add_connection(UcxConnection *conn); | ||
|
|
||
| bool remove_connection(UcxConnection *conn); |
There was a problem hiding this comment.
return value is not handled anywhere now, can be void with new virtual func or maybe assert for debug
| virtual void dispatch_connection_error(UcxConnection *conn) { | ||
| LOG << "deleting connection with status " | ||
| << ucs_status_string(conn->ucx_status()); | ||
| --_curr_state.active_conns; |
There was a problem hiding this comment.
can server be an initiator of disconnect?
|
|
||
| virtual bool remove_connection(UcxConnection *conn); | ||
| // Called when new server connection is accepted | ||
| virtual void dispatch_connection_accepted(UcxConnection* conn); |
There was a problem hiding this comment.
IMO better to avoid functions in base class which are relevant only for one inheritor, add_conection looks more generic
There was a problem hiding this comment.
add/remove connection was intended to be private helper to manage conn_id hash and not virtual
When QP is modified by DevX, the 'state' field may not be updated
Reserve a CQ entry for NOP operation during RC QP flush. Only one is needed since this is a blocking operation. This removes the need to disable TX moderation to avoid deadlock during flush due to lack of CQ resources to post a single NOP (which is needed to gen completions for unsignaled WQEs)
The client counts incorrectly the number of outstanding ops: when a connection is closed, _num_sent is decreased, but it should not be, since read requests also complete with error status and _num_completions is increased. As a result, the client sends more and more outstanding operations after every terminated connection, which causes increase in memory usage. The fix is to first close the connection, which will restore completions for read requests, and then restore the remaining credits for write requests - which will never complete on their own because they are waiting for write completion io-message.
yosefe
force-pushed
the
topic/rc-and-iodemo-fixes
branch
from
fe6cc0f
to
e059cfb
Compare
evgeny-leksikov
approved these changes
Oct 27, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.