UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count #7093

dmitrygx · 2021-07-13T11:06:52Z

What

Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count.

Why ?

Fixes #6705.
Fixes #6471.
When the large amount of connections are started simultaneously, it leads to network congestions and some TCP EPs are failed, because they don't receive ACK on previously sent KEEPALIVE packet. Increasing timeout helps to resolve the issue. 1s timeout between retries is too small one.

How ?

Change value of UCT_TCP_EP_DEFAULT_KEEPALIVE_INTVL from 1 to 2.
Update KEEPCNT configuration parameter to be ULUNITS and set "auto" as a default value. Handle it when setting socket options for KA.

yosefe · 2021-07-13T12:20:37Z

src/uct/tcp/tcp.h

@@ -65,7 +65,7 @@
 #define UCT_TCP_EP_DEFAULT_KEEPALIVE_IDLE    10

 /* The seconds between individual keepalive probes */
-#define UCT_TCP_EP_DEFAULT_KEEPALIVE_INTVL   1
+#define UCT_TCP_EP_DEFAULT_KEEPALIVE_INTVL   10


shouldn't TCP get the interval from UCP layer?

shouldn't TCP get the interval from UCP layer?

we should get the interval from UCP layer for UCT_TCP_EP_DEFAULT_KEEPALIVE_IDLE - this parameter configures interval for starting KA if TCP socket is idle

what is the difference between them? maybe we should set both?

what is the difference between them? maybe we should set both?

UCT_TCP_EP_DEFAULT_KEEPALIVE_INTVL is interval between retries when the KA started.
UCT_TCP_EP_DEFAULT_KEEPALIVE_IDLE is interval between last packet with user data and first KA message. The first KA message is sent if socket is idle.

after KEEPALIVE_IDLE timeout kernel starts ping peer every KEEPALIVE_INTVL seconds till get response, after this kernel waits for KEEPALIVE_IDLE again

after KEEPALIVE_IDLE timeout kernel starts ping peer every KEEPALIVE_INTVL seconds till get response, after this kernel waits for KEEPALIVE_IDLE again

so it seems that if we set both IDLE and INVL to same value as UCP keepalive interval, the behavior will be similar to other transports which use ep_check, where each ep_check is sent after same amount of time

ok, I agree. Let's implement through extending uct_iface_open() API for built-in keepalive case?

right, but here is issue: UCP KA may stop after N endpoints, but TCP KA will ping all EPs simultaneously.
that is why we didn't merge these values before.
not sure if this is issue, but there is different behaviour for timeouts

UCP KA may stop after N endpoints, but TCP KA will ping all EPs simultaneously.

I see, this can happen

ucx/src/uct/api/uct.h

Line 1097 in b75809f

ucs_time_t keepalive_interval;

yosefe · 2021-07-14T12:49:27Z

src/uct/tcp/tcp.h

@@ -440,7 +440,7 @@ typedef struct uct_tcp_iface_config {
    ucs_range_spec_t               port_range;
    struct {
        ucs_time_t                 idle;
-        unsigned                   cnt;
+        size_t                     cnt;


unsigned long

yosefe · 2021-07-14T12:49:33Z

src/uct/tcp/tcp.h

@@ -402,7 +402,7 @@ typedef struct uct_tcp_iface {
            ucs_time_t            idle;              /* The time the connection needs to remain
                                                      * idle before TCP starts sending keepalive
                                                      * probes (TCP_KEEPIDLE socket option) */
-            unsigned              cnt;               /* The maximum number of keepalive probes TCP
+            size_t                cnt;               /* The maximum number of keepalive probes TCP


unsigned long

yosefe · 2021-07-14T12:50:02Z

src/uct/tcp/tcp_iface.c

@@ -872,7 +872,7 @@ int uct_tcp_keepalive_is_enabled(uct_tcp_iface_t *iface)
 {
 #ifdef UCT_TCP_EP_KEEPALIVE
    return (iface->config.keepalive.idle != UCS_TIME_INFINITY) &&
-           (iface->config.keepalive.cnt != 0) &&
+           (iface->config.keepalive.cnt != UCS_ULUNITS_INF) &&


why changed?

since, 0 is valid value and means no need to retry, fail immediately.
we should use inf as for other configuration parameters.

need to document this in config param desc?

yosefe · 2021-07-14T14:06:13Z

src/uct/tcp/tcp_ep.c

    if (time_val == UCS_TIME_AUTO) {
        return auto_val;
    }

-    return ucs_max(1, (int)ucs_time_to_sec(time_val));
+    time_sec = ucs_time_to_sec(time_val);
+    if ((time_val == UCS_TIME_INFINITY) || (time_sec >= (double)INT_MAX)) {


if (time_val == AUTO) return AUTO_VAL } else if (time_val == INFINITY) return INT_MAX } else { return ucs_min(INT_MAX, ucs_max(1l, ucs_time_to_sec(time_val)); }

yosefe · 2021-07-14T14:07:10Z

src/uct/tcp/tcp_ep.c

@@ -527,9 +535,13 @@ static ucs_status_t uct_tcp_ep_keepalive_enable(uct_tcp_ep_t *ep)
    }

    if (iface->config.keepalive.cnt != UCS_ULUNITS_AUTO) {
+        if ((iface->config.keepalive.cnt == UCS_ULUNITS_INF) ||
+            (iface->config.keepalive.cnt >= (unsigned long)keepalive_cnt)) {


seems keepalive_cnt can be un initialized here?

good catch, fixed

yosefe · 2021-07-14T15:02:52Z

src/uct/tcp/tcp_ep.c

    if (time_val == UCS_TIME_AUTO) {
        return auto_val;
+    } else if (time_val == UCS_TIME_INFINITY) {
+        return INT16_MAX;


maybe pass max_val?

yosefe · 2021-07-14T15:03:39Z

src/uct/tcp/tcp_ep.c

+            (iface->config.keepalive.cnt > INT8_MAX)) {
            keepalive_cnt = INT8_MAX;
+        } else {
+            keepalive_cnt = (uint8_t)iface->config.keepalive.cnt;


keepalive_cnt = ucs_min(INT8_MAX, iface->config.keepalive.cnt)

dmitrygx · 2021-07-14T19:24:18Z

@yosefe is it ok now?

…em default for keepalive retries count

brminich approved these changes Jul 13, 2021

View reviewed changes

yosefe reviewed Jul 13, 2021

View reviewed changes

dmitrygx force-pushed the topic/uct/tcp_ka branch from 939f3b0 to 7b3112d Compare July 14, 2021 12:09

dmitrygx changed the title ~~UCT/TCP: Increase keepalive interval from 1 to 10 seconds~~ UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count Jul 14, 2021

yosefe reviewed Jul 14, 2021

View reviewed changes

yosefe approved these changes Jul 14, 2021

View reviewed changes

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use syst…

f5ade86

…em default for keepalive retries count

dmitrygx force-pushed the topic/uct/tcp_ka branch from 757a53f to f5ade86 Compare July 14, 2021 20:04

dmitrygx merged commit 336f6e5 into openucx:master Jul 15, 2021

dmitrygx mentioned this pull request Jul 17, 2021

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count [v1.11.x] #7117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count #7093

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count #7093

dmitrygx commented Jul 13, 2021 •

edited

Loading

yosefe Jul 13, 2021

dmitrygx Jul 13, 2021

yosefe Jul 13, 2021

dmitrygx Jul 13, 2021 •

edited

Loading

hoopoepg Jul 13, 2021

yosefe Jul 13, 2021

dmitrygx Jul 13, 2021

hoopoepg Jul 13, 2021

dmitrygx Jul 13, 2021

yosefe Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

yosefe Jul 14, 2021

dmitrygx Jul 14, 2021

dmitrygx commented Jul 14, 2021

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count #7093

UCT/TCP: Increase keepalive interval from 1 to 2 seconds and use system default for keepalive retries count #7093

Conversation

dmitrygx commented Jul 13, 2021 • edited Loading

What

Why ?

How ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrygx Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrygx commented Jul 14, 2021

dmitrygx commented Jul 13, 2021 •

edited

Loading

dmitrygx Jul 13, 2021 •

edited

Loading