[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADVnQymw1A18bgGX=ujOmsVijQrfvSSnmtwRAqdx_JWCeNnvnw@mail.gmail.com>
Date: Wed, 3 Nov 2021 17:53:08 -0400
From: Neal Cardwell <ncardwell@...gle.com>
To: Yuchung Cheng <ycheng@...gle.com>
Cc: Akhmat Karakotov <hmukos@...dex-team.ru>, kafai@...com,
brakmo@...com, eric.dumazet@...il.com, mitradir@...dex-team.ru,
netdev@...r.kernel.org, zeil@...dex-team.ru
Subject: Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO
On Wed, Nov 3, 2021 at 5:23 PM Yuchung Cheng <ycheng@...gle.com> wrote:
>
> On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov <hmukos@...dex-team.ru> wrote:
> >
> > 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 <hmukos@...dex-team.ru>
> > ---
> > 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
Powered by blists - more mailing lists