Message-ID: <CANn89iJsC73o9hJ_RUd9qfv50ebt2H5VZx0-xgrPXFAZVWeGgQ@mail.gmail.com>
Date:   Fri, 10 Jul 2020 08:20:03 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.co.jp>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        Kuniyuki Iwashima <kuni1840@...il.com>,
        Benjamin Herrenschmidt <benh@...zon.com>,
        osa-contribution-log@...zon.com, Julian Anastasov <ja@....bg>
Subject: Re: [PATCH v3 net-next] inet: Remove an unnecessary argument of syn_ack_recalc().

On Fri, Jul 10, 2020 at 7:11 AM Kuniyuki Iwashima <kuniyu@...zon.co.jp> wrote:
>
> Commit 0c3d79bce48034018e840468ac5a642894a521a3 ("tcp: reduce SYN-ACK
> retrans for TCP_DEFER_ACCEPT") introduces syn_ack_recalc() which decides
> if a minisock is held and a SYN+ACK is retransmitted or not.
>
> If rskq_defer_accept is not zero in syn_ack_recalc(), max_retries always
> has the same value because max_retries is overwritten by rskq_defer_accept
> in reqsk_timer_handler().
>
> This commit adds three changes:
> - remove redundant non-zero check for rskq_defer_accept in
>    reqsk_timer_handler().
> - remove max_retries from the arguments of syn_ack_recalc() and use
>    rskq_defer_accept instead.
> - rename thresh to max_syn_ack_retries for readability.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.co.jp>
> Reviewed-by: Benjamin Herrenschmidt <benh@...zon.com>
> CC: Julian Anastasov <ja@....bg>
> ---
>  net/ipv4/inet_connection_sock.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index afaf582a5aa9..21bc80a3c7cf 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -648,20 +648,23 @@ struct dst_entry *inet_csk_route_child_sock(const struct sock *sk,
>  EXPORT_SYMBOL_GPL(inet_csk_route_child_sock);
>
>  /* Decide when to expire the request and when to resend SYN-ACK */
> -static inline void syn_ack_recalc(struct request_sock *req, const int thresh,
> -                                 const int max_retries,
> +static inline void syn_ack_recalc(struct request_sock *req,

While we are at it, please remove the inline keyword.

> +                                 const int max_syn_ack_retries,
>                                   const u8 rskq_defer_accept,
>                                   int *expire, int *resend)
>  {
>         if (!rskq_defer_accept) {
> -               *expire = req->num_timeout >= thresh;
> +               *expire = req->num_timeout >= max_syn_ack_retries;
>                 *resend = 1;
>                 return;
>         }
> -       *expire = req->num_timeout >= thresh &&
> -                 (!inet_rsk(req)->acked || req->num_timeout >= max_retries);
> -       /*
> -        * Do not resend while waiting for data after ACK,
> +       /* If a bare ACK has already been dropped, the client is alive, so
> +        * do not free the request_sock to drop a bare ACK at most
> +        * rskq_defer_accept times and wait for data.
> +        */

I honestly do not believe this comment is needed.
The bare ack has not been 'dropped' since it had the effect of
validating the 3WHS.
I find it confusing, and not describing the order of the conditions
expressed in the C code.

> +       *expire = req->num_timeout >= max_syn_ack_retries &&
> +                 (!inet_rsk(req)->acked || req->num_timeout >= rskq_defer_accept);
> +       /* Do not resend while waiting for data after ACK,
>          * start to resend on end of deferring period to give
>          * last chance for data or ACK to create established socket.
>          */
> @@ -720,15 +723,12 @@ static void reqsk_timer_handler(struct timer_list *t)
>         struct net *net = sock_net(sk_listener);
>         struct inet_connection_sock *icsk = inet_csk(sk_listener);
>         struct request_sock_queue *queue = &icsk->icsk_accept_queue;
> -       int qlen, expire = 0, resend = 0;
> -       int max_retries, thresh;
> -       u8 defer_accept;
> +       int max_syn_ack_retries, qlen, expire = 0, resend = 0;
>
>         if (inet_sk_state_load(sk_listener) != TCP_LISTEN)
>                 goto drop;
>
> -       max_retries = icsk->icsk_syn_retries ? : net->ipv4.sysctl_tcp_synack_retries;
> -       thresh = max_retries;
> +       max_syn_ack_retries = icsk->icsk_syn_retries ? : net->ipv4.sysctl_tcp_synack_retries;
>         /* Normally all the openreqs are young and become mature
>          * (i.e. converted to established socket) for first timeout.
>          * If synack was not acknowledged for 1 second, it means
> @@ -750,17 +750,14 @@ static void reqsk_timer_handler(struct timer_list *t)
>         if ((qlen << 1) > max(8U, READ_ONCE(sk_listener->sk_max_ack_backlog))) {
>                 int young = reqsk_queue_len_young(queue) << 1;
>
> -               while (thresh > 2) {
> +               while (max_syn_ack_retries > 2) {
>                         if (qlen < young)
>                                 break;
> -                       thresh--;
> +                       max_syn_ack_retries--;
>                         young <<= 1;
>                 }
>         }
> -       defer_accept = READ_ONCE(queue->rskq_defer_accept);
> -       if (defer_accept)
> -               max_retries = defer_accept;
> -       syn_ack_recalc(req, thresh, max_retries, defer_accept,
> +       syn_ack_recalc(req, max_syn_ack_retries, READ_ONCE(queue->rskq_defer_accept),
>                        &expire, &resend);
>         req->rsk_ops->syn_ack_timeout(req);
>         if (!expire &&
> --
> 2.17.2 (Apple Git-113)
>

Powered by blists - more mailing lists