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: <20180703072113.19910-1-jmaxwell37@gmail.com>
In-Reply-To: <20180703072113.19910-1-jmaxwell37@gmail.com>
From:   Neal Cardwell <ncardwell@google.com>
Date:   Tue, 3 Jul 2018 10:13:07 -0400
Message-ID: <CADVnQymzHMfjLKRRu0AmYa6PX6YXzyZ-2sF9TjyVfLZs6GVZTw@mail.gmail.com>
Subject: Re: [net-next,v1] 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 3:21 AM Jon Maxwell <jmaxwell37@gmail.com> wrote:
>
> v1 contains the following suggestions by Neal Cardwell:
>
> 1) Fix up units mismatch regarding msec/jiffies.
> 2) Address possiblility of time_remaining being negative.
> 3) Add a helper routine to do the rto calculation.
>
> Every time the TCP retransmission timer fires. It checks to see if there is a
> timeout before scheduling the next retransmit timer. The retransmit interval
> between each retransmission increases exponentially. The issue is that in order
> for the timeout to occur the retransmit timer needs to fire again. If the user
> timeout check happens after the 9th retransmit for example. It needs to wait for
> the 10th retransmit timer to fire in order to evaluate whether a timeout has
> occurred or not. If the interval is large enough then the timeout will be
> inaccurate.
>
> For example with a TCP_USER_TIMEOUT of 10 seconds without patch:
>
> 1st retransmit:
>
> 22:25:18.973488 IP host1.49310 > host2.search-agent: Flags [.]
>
> Last retransmit:
>
> 22:25:26.205499 IP host1.49310 > host2.search-agent: Flags [.]
>
> Timeout:
>
> send: Connection timed out
> Sun Jul  1 22:25:34 EDT 2018
>
> We can see that last retransmit took ~7 seconds. Which pushed the total
> timeout to ~15 seconds instead of the expected 10 seconds. This gets more
> inaccurate the larger the TCP_USER_TIMEOUT value. As the interval increases.
>
> Add tcp_clamp_rto_to_user_timeout() to determine if the user rto has expired.
> Or whether the rto interval needs to be recalculated. Use the original interval
> if user rto is not set.
>
> Test results with the patch is the expected 10 second timeout:
>
> 1st retransmit:
>
> 01:37:59.022555 IP host1.49310 > host2.search-agent: Flags [.]
>
> Last retransmit:
>
> 01:38:06.486558 IP host1.49310 > host2.search-agent: Flags [.]
>
> Timeout:
>
> send: Connection timed out
> Mon Jul  2 01:38:09 EDT 2018
>
> Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
> ---
>  net/ipv4/tcp_timer.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 3b3611729928..82c2a3b3713c 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -22,6 +22,23 @@
>  #include <linux/gfp.h>
>  #include <net/tcp.h>
>
> +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;
> +
> +       if (!icsk->icsk_user_timeout)
> +               return rto;
> +       elapsed = tcp_time_stamp(tcp_sk(sk)) - tcp_sk(sk)->retrans_stamp;

Thanks. The local logic seems OK to me now, but from reading
retransmits_timed_out() it looks like at this point in the code we are
not guaranteed that tcp_sk(sk)->retrans_stamp is initialized to
something non-zero. So we probably need a preceding preparatory patch
that factors out the first few lines of retransmits_timed_out()  into
a helper frunction to get the start_ts for use in this calculation.
Perhaps:

u32 tcp_retrans_stamp():
       start_ts = tcp_sk(sk)->retrans_stamp;
       if (unlikely(!start_ts)) {
               head = tcp_rtx_queue_head(sk);
               if (!head)
                       return 0;
               start_ts = tcp_skb_timestamp(head);
       }
       return start_ts;

And then the new tcp_clamp_rto_to_user_timeout() can use the helper:

...
retrans_stamp = tcp_retransmit_stamp(sk);
if (!retrans_stamp)
   return rto;
elapsed = tcp_time_stamp(tcp_sk(sk)) - retrans_stamp;
...

Eric wrote those lines to recalculate start_ts, so we may want to wait
until Eric returns to review this before merging the resulting patch
series.

neal