Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

On Wed, Nov 3, 2021 at 5:23 PM Yuchung Cheng [off-list ref] wrote:

On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov [off-list ref] wrote:

quoted

When setting RTO through BPF program, some SYN ACK packets were unaffected
and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
and is reassigned through BPF using tcp_timeout_init call. SYN ACK
retransmits now use newly added timeout option.

Signed-off-by: Akhmat Karakotov <redacted>
---
 include/net/request_sock.h      |  2 ++
 include/net/tcp.h               |  2 +-
 net/ipv4/inet_connection_sock.c |  4 +++-
 net/ipv4/tcp_input.c            |  8 +++++---
 net/ipv4/tcp_minisocks.c        | 12 +++++++++---
 5 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 29e41ff3ec93..144c39db9898 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h

@@ -70,6 +70,7 @@ struct request_sock {
        struct saved_syn                *saved_syn;
        u32                             secid;
        u32                             peer_secid;
+       u32                             timeout;
 };

 static inline struct request_sock *inet_reqsk(const struct sock *sk)

@@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
        sk_node_init(&req_to_sk(req)->sk_node);
        sk_tx_queue_clear(req_to_sk(req));
        req->saved_syn = NULL;
+       req->timeout = 0;
        req->num_timeout = 0;
        req->num_retrans = 0;
        req->sk = NULL;

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3166dc15d7d6..e328d6735e38 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h

@@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)

        if (timeout <= 0)
                timeout = TCP_TIMEOUT_INIT;
-       return timeout;
+       return min_t(int, timeout, TCP_RTO_MAX);
 }

 static inline u32 tcp_rwnd_init_bpf(struct sock *sk)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 0d477c816309..cdf16285e193 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c

@@ -870,7 +870,9 @@ static void reqsk_timer_handler(struct timer_list *t)

                if (req->num_timeout++ == 0)
                        atomic_dec(&queue->young);
-               timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
+               timeo = min_t(unsigned long,
+                             (unsigned long)req->timeout << req->num_timeout,
+                             TCP_RTO_MAX);

would it make sense to have a helper tcp_timeout_max() to reduce
clutter? but that can be done by a later refactor patch

I like Yuchung's idea to have a helper function (perhaps
reqsk_timeout() to go with reqsk_timer_handler()?)  to calculate the
timeout value, since there are 3 of these non-trivial expressions.
Otherwise this looks good to me.

thanks,
neal