Re: [net-next,v1] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy

[Jonathan Maxwell]

On Wed, Jul 4, 2018 at 12:13 AM, Neal Cardwell <ncardwell@xxxxxxxxxx> wrote:
> On Tue, Jul 3, 2018 at 3:21 AM Jon Maxwell <jmaxwell37@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>  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.
>

You are right. tcp_clamp_rto_to_user_timeout() should do the
same check as retransmits_timed_out() in regards to
tcp_sk(sk)->retrans_stamp. I'll add that to v2/test and then Eric can
comment on that if he has any input when he returns.

Thanks for all your help.

Regards

Jon

> neal