tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
Subsystems (3)
PATCH v2 1/1 tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
v2 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 tcp_clamp_rto_to_user_timeout() to do the rto calculation.
4) Move start_ts logic into helper routine tcp_retrans_stamp() to validate tcp_sk(sk)->retrans_stamp.
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
--- net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-)
net/ipv4/tcp_timer.c +39 -9
@@ -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; + +if (unlikely (!start_ts)) { +struct sk_buff *head =tcp_rtx_queue_head (sk); + +if (!head) +return 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; + + 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)); +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);
Signature · 1 line
2.13.6
Neal Cardwell
maintainerQuoted 11 lines · Jon Maxwell
Signed-off-by Jon Maxwell <[email protected]>--- net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-)net/ipv4/tcp_timer.c +0
-0
net/ipv4/tcp_timer.c line 27
Quoted 6 lines
net/ipv4/tcp_timer.c +3
-0@@ -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.
net/ipv4/tcp_timer.c line 33
Quoted 6 lines
net/ipv4/tcp_timer.c +6
-0+ +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.
net/ipv4/tcp_timer.c line 44
Quoted 11 lines
net/ipv4/tcp_timer.c +11
-0+ 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).
net/ipv4/tcp_timer.c line 54
Quoted 10 lines
net/ipv4/tcp_timer.c +10
-0+ + 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));
Quoted 33 lines
net/ipv4/tcp_timer.c +7
-8+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 &&
net/ipv4/tcp_timer.c void tcp_retransmit_timer(struct sock *sk) · line 570
Quoted 8 lines
net/ipv4/tcp_timer.c +2
-1@@ -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
From: Neal Cardwell
Quoted 13 lines
Sent: 04 July 2018 15:14
Quoted 10 lines · Jon Maxwell
Signed-off-by Jon Maxwell <[email protected]>--- net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-)net/ipv4/tcp_timer.c +0
-0
...
Quoted 4 lines
net/ipv4/tcp_timer.c line 42
Quoted 4 lines
net/ipv4/tcp_timer.c +4
-0+static __u32 tcp_clamp_rto_to_user_timeout (struct sock *sk) +{ +struct inet_connection_sock *icsk =inet_csk (sk); +__u32 rto = icsk->icsk_rto;
Why cache rto past all the function calls until it is needed?
Quoted 16 lines
net/ipv4/tcp_timer.c line 44
Quoted 2 lines
net/ipv4/tcp_timer.c +2
-0+__u32 elapsed, user_timeout; +unsigned int start_ts;I'd suggest u32 here for
start_ts(per the rationale above).net/ipv4/tcp_timer.c line 54
Quoted 10 lines
net/ipv4/tcp_timer.c +10
-0+ + 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));
Quoted 3 lines
My sense is that
min_twould be preferred here, e.g:rto = min_t(__u32, rto, msecs_to_jiffies(user_timeout - elapsed));
Think I'd just write the conditional...
Quoted 2 lines
net/ipv4/tcp_timer.c
Quoted 2 lines
net/ipv4/tcp_timer.c +2
-0+return rto; +}
Looks to me like it doesn't correctly allow for the conversions between millisecond and jiffies either.
I suspect the msecs_to_jiffies() call can return zero (unless it rounds up). Coding with a signed 'time until user timeout' might make it simpler.
David
Let's wait for Eric to review. Then I'll put together the next version.
Regards
Jon
Quoted 55 lines · David Laight
From: Neal Cardwell
Quoted 13 lines
Sent: 04 July 2018 15:14
Quoted 10 lines · Jon Maxwell
Signed-off-by Jon Maxwell <[email protected]>--- net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-)net/ipv4/tcp_timer.c +0
-0...
Quoted 4 lines
net/ipv4/tcp_timer.c line 42
Quoted 4 lines
net/ipv4/tcp_timer.c +4
-0+static __u32 tcp_clamp_rto_to_user_timeout (struct sock *sk) +{ +struct inet_connection_sock *icsk =inet_csk (sk); +__u32 rto = icsk->icsk_rto;Why cache rto past all the function calls until it is needed?
Quoted 16 lines
net/ipv4/tcp_timer.c line 44
Quoted 2 lines
net/ipv4/tcp_timer.c +2
-0+__u32 elapsed, user_timeout; +unsigned int start_ts;I'd suggest u32 here for
start_ts(per the rationale above).net/ipv4/tcp_timer.c line 54
Quoted 10 lines
net/ipv4/tcp_timer.c +10
-0+ + 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));Quoted 3 lines
My sense is that
min_twould be preferred here, e.g:rto = min_t(__u32, rto, msecs_to_jiffies(user_timeout - elapsed));Think I'd just write the conditional...
Quoted 2 lines
net/ipv4/tcp_timer.c
Quoted 2 lines
net/ipv4/tcp_timer.c +2
-0+return rto; +}Looks to me like it doesn't correctly allow for the conversions between millisecond and jiffies either.
I suspect themsecs_to_jiffies()call can return zero (unless it rounds up). Coding with a signed 'time until user timeout' might make it simpler.David
Quoted 1 line · Jonathan Maxwell
Let's wait for Eric to review. Then I'll put together the next version.
Sorry for the delay (I was travelling last week) , please respin a v3, thanks !