Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180704000608.17360-1-jmaxwell37@gmail.com>
In-Reply-To: <20180704000608.17360-1-jmaxwell37@gmail.com>
From:   Neal Cardwell <ncardwell@google.com>
Date:   Wed, 4 Jul 2018 10:13:56 -0400
Message-ID: <CADVnQykfvmWyOJxT7_nHPOvfNEwi7+Hz2qJPooPpNLmdMWChYw@mail.gmail.com>
Subject: Re: [net-next,v2] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
To:     Jonathan Maxwell <jmaxwell37@gmail.com>
Cc:     David Miller <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, jmaxwell@redhat.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 3, 2018 at 8:06 PM Jon Maxwell <jmaxwell37@gmail.com> wrote:
>
> Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
> ---
>  net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 3b3611729928..d129e670d02a 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -22,6 +22,39 @@
>  #include <linux/gfp.h>
>  #include <net/tcp.h>
>
> +unsigned int tcp_retransmit_stamp(struct sock *sk)
> +{
> +       unsigned int start_ts = tcp_sk(sk)->retrans_stamp;

Since retrans_stamp and tcp_skb_timestamp() are both u32, I'd suggest
using u32 for the local variable start_ts and the return type of the
function.

> +
> +       if (unlikely(!start_ts)) {
> +               struct sk_buff *head = tcp_rtx_queue_head(sk);
> +
> +               if (!head)
> +                       return false;

Looks like a copy-and-paste holdover: since this function is returning
an integer, I would suggest returning 0 rather than false.

> +               start_ts = tcp_skb_timestamp(head);
> +       }
> +       return start_ts;
> +}
> +
> +static __u32 tcp_clamp_rto_to_user_timeout(struct sock *sk)
> +{
> +       struct inet_connection_sock *icsk = inet_csk(sk);
> +       __u32 rto = icsk->icsk_rto;
> +       __u32 elapsed, user_timeout;
> +       unsigned int start_ts;

I'd suggest u32 here for start_ts (per the rationale above).

> +
> +       start_ts = tcp_retransmit_stamp(sk);
> +       if (!icsk->icsk_user_timeout || !start_ts)
> +               return rto;
> +       elapsed = tcp_time_stamp(tcp_sk(sk)) - start_ts;
> +       user_timeout = jiffies_to_msecs(icsk->icsk_user_timeout);
> +       if (elapsed >= user_timeout)
> +               rto = 1;  /* user timeout has passed; fire ASAP */
> +       else
> +               rto = min(rto, (__u32)msecs_to_jiffies(user_timeout - elapsed));

My sense is that min_t would be preferred here, e.g:

  rto = min_t(__u32, rto, msecs_to_jiffies(user_timeout - elapsed));

> +       return rto;
> +}
> +
>  /**
>   *  tcp_write_err() - close socket and save error info
>   *  @sk:  The socket the error has appeared on.
> @@ -166,14 +199,9 @@ static bool retransmits_timed_out(struct sock *sk,
>         if (!inet_csk(sk)->icsk_retransmits)
>                 return false;
>
> -       start_ts = tcp_sk(sk)->retrans_stamp;
> -       if (unlikely(!start_ts)) {
> -               struct sk_buff *head = tcp_rtx_queue_head(sk);
> -
> -               if (!head)
> -                       return false;
> -               start_ts = tcp_skb_timestamp(head);
> -       }
> +       start_ts = tcp_retransmit_stamp(sk);
> +       if (!start_ts)
> +               return false;
>
>         if (likely(timeout == 0)) {
>                 linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base);
> @@ -407,6 +435,7 @@ void tcp_retransmit_timer(struct sock *sk)
>         struct tcp_sock *tp = tcp_sk(sk);
>         struct net *net = sock_net(sk);
>         struct inet_connection_sock *icsk = inet_csk(sk);
> +       __u32 rto;
>
>         if (tp->fastopen_rsk) {
>                 WARN_ON_ONCE(sk->sk_state != TCP_SYN_RECV &&
> @@ -535,7 +564,8 @@ void tcp_retransmit_timer(struct sock *sk)
>                 /* Use normal (exponential) backoff */
>                 icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX);
>         }
> -       inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto, TCP_RTO_MAX);
> +       rto = tcp_clamp_rto_to_user_timeout(sk);
> +       inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, rto, TCP_RTO_MAX);
>         if (retransmits_timed_out(sk, net->ipv4.sysctl_tcp_retries1 + 1, 0))
>                 __sk_dst_reset(sk);


Thanks!
neal