Skip to content

UCT/IB/UD: Introduce EP check (Keep Alive) - #6080

Merged
yosefe merged 1 commit into
openucx:masterfrom
dmitrygx:topic/uct/ud_ep_check
Jan 1, 2021
Merged

yosefe merged 1 commit into
openucx:masterfrom
dmitrygx:topic/uct/ud_ep_check

Conversation

@dmitrygx

Copy link
Copy Markdown
Member

What

Introduce EP check (Keep Alive) in UCT/UD.

Why ?

To extend the functionality provided by UCT/UD and use it in UCP for better error handling.

How ?

  1. Introduce new packet type - UCT_UD_PACKET_FLAG_CHECK.
  2. The packet is sent by UD when a user calls uct_ep_check() (it is not sent if UD Ep is not connected or there are some unacked TX operations).
  3. Upon receiving the packet, UD increases PSN that will be ACKed then and ACKs its own operations (if they exist).

@dmitrygx

dmitrygx commented Dec 31, 2020 •

Copy link
Copy Markdown
Member Author

@hoopoepg @rakhmets could you review pls?

Comment thread src/uct/ib/ud/base/ud_def.h Outdated
UCT_UD_PACKET_FLAG_NAK = UCS_BIT(27),
UCT_UD_PACKET_FLAG_PUT = UCS_BIT(28),
UCT_UD_PACKET_FLAG_CTL = UCS_BIT(29),
UCT_UD_PACKET_FLAG_CHECK = UCS_BIT(30),

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.

no need for special flag, it should be just ACK+ACK_REQ

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.

i'm not sure that it would work (e.g. in case of user doesn't do AM/PUT operations), since we should increase local psn, and get ACKs on it

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.

yes, I checked, it doesn't work
udx/test_ucp_peer_failure_keepalive.kill_receiver/0 doesn't detect an error, because acked_psn == psn - 1 - so UD considers peer as alive.

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.

ic, need to psn++, so can it be a PUT message?

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.

so can it be a PUT message?

do you mean to implement the same scheme as in RC? but it will consume receive WQ element on peer
I guess it was intent on Rc to not consume and repost receive WQ element

Comment thread src/uct/ib/ud/base/ud_ep.c Outdated
Comment on lines +1119 to +1127
skb = uct_ud_ep_prepare_check(ep);
if (skb == NULL) {
/* schedule keepalive request to pending queue for futher sending */
uct_ud_ep_ctl_op_add(iface, ep, UCT_UD_EP_OP_CHECK);
goto out;
}

uct_ud_iface_send_ctl(iface, ep, skb, NULL, 0, 0, 1);
uct_ud_iface_complete_tx_skb(iface, ep, skb);

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.

just need to add ctl_op, and let it be sent from progress

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

@dmitrygx

Copy link
Copy Markdown
Member Author

@yosefe replaced by doing PUT in EP Check instead of a new operation. much easier :) thanks for the advise!

Comment thread src/uct/ib/ud/base/ud_ep.c Outdated
Comment on lines +1077 to +1078
if (/* check that TX resources are available (i.e. there are no signaled
* operations which provide actual peer status) */

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.

ud_enter
if (!connected || !last_ack_recvd) {
   ud_leave();
   return OK;
}
ud_leave()

return uct_ep_put_short(...)

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.

yes, good catch
fixed

@dmitrygx
dmitrygx force-pushed the topic/uct/ud_ep_check branch from 3c6d46a to d19a639 Compare December 31, 2020 17:29
@yosefe
yosefe merged commit 918d01e into openucx:master Jan 1, 2021
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.

3 participants