| Message ID | 20210723093938.49354-1-zeil@yandex-team.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: use rto_min value from socket in retransmits timeout | expand |
.(On Fri, Jul 23, 2021 at 5:41 AM Dmitry Yakunin <zeil@yandex-team.ru> wrote: > > Commit ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") > adds ability to set rto_min value on socket less then default TCP_RTO_MIN. > But retransmits_timed_out() function still uses TCP_RTO_MIN and > tcp_retries{1,2} sysctls don't work properly for tuned socket values. > > Fixes: ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") > Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru> > Acked-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru> > --- > net/ipv4/tcp_timer.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > index 20cf4a9..66c4b97 100644 > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -199,12 +199,13 @@ static unsigned int tcp_model_timeout(struct sock *sk, > * @boundary: max number of retransmissions > * @timeout: A custom timeout value. > * If set to 0 the default timeout is calculated and used. > - * Using TCP_RTO_MIN and the number of unsuccessful retransmits. > + * Using icsk_rto_min value from socket or RTAX_RTO_MIN from route > + * and the number of unsuccessful retransmits. > * > * The default "timeout" value this function can calculate and use > * is equivalent to the timeout of a TCP Connection > * after "boundary" unsuccessful, exponentially backed-off > - * retransmissions with an initial RTO of TCP_RTO_MIN. > + * retransmissions with an initial RTO of icsk_rto_min or RTAX_RTO_MIN. > */ > static bool retransmits_timed_out(struct sock *sk, > unsigned int boundary, > @@ -217,7 +218,7 @@ static bool retransmits_timed_out(struct sock *sk, > > start_ts = tcp_sk(sk)->retrans_stamp; > if (likely(timeout == 0)) { > - unsigned int rto_base = TCP_RTO_MIN; > + unsigned int rto_base = tcp_rto_min(sk); > > if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) > rto_base = tcp_timeout_init(sk); > -- I would argue strenuously against this. We tried the approach in this patch at Google years ago, but we had to revert that approach and go back to using TCP_RTO_MIN as the baseline for the computation, because using the custom tcp_rto_min(sk) caused serious reliability problems. The behavior in this patch causes various serious reliability problems because the retransmits_timed_out() computation is used for various timeout decisions that determine how long a connection tries to retransmit something before deciding the path is bad and/or giving up and closing the connection. Here are a few of the problems this causes: (1) The biggest one is probably orphan retries. By default tcp_orphan_retries() uses a retry count of 8. But if your min_rto is 5ms (used at Google for many years), then the 8 retries means an orphaned connection (whose fd is no longer held by a process, but is still established) only lasts for 1.275 seconds before giving up and closing. This means that connectivity problems longer than 1.275 seconds (extremely common with cellular links) are not tolerated for such connections; the connections often do not receive the data they were supposed to receive. (2) TCP_RETR1 /sysctl_tcp_retries1, used for __dst_negative_advice(), also has big problems. Even with a min_rto as big as 20ms, on a route with 150ms RTT, the approach in this patch will cause retransmits_timed_out() to return true upon the 1st RTO timer firing, even though TCP_RETR1 is 3. (3) TCP_RETR2 /sysctl_tcp_retries2, with a default of 15, used for regular connection retry lifetimes, also has a massive decrease in robustness, due to falling from 109 minutes with a 200ms RTO, to about 2.7 minutes with a min_rto of 5ms. neal
Hello, Neal! Thanks for your reply and explanations. I agree with all your points, about safe defaults for both timeouts and the number of retries. But what the patch does is not changing the defaults, it only provides a way to work with these values through bpf, which is important in an environment that is way different from cellular networks. For example in the modern DC the rto_min value should correspond with real RTT, that definitely not 200ms. Also I add Alexander Azimov for further discussions. -- Dmitry > On 23 Jul 2021, at 17:41, Neal Cardwell <ncardwell@google.com> wrote: > > .(On Fri, Jul 23, 2021 at 5:41 AM Dmitry Yakunin <zeil@yandex-team.ru> wrote: >> >> Commit ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") >> adds ability to set rto_min value on socket less then default TCP_RTO_MIN. >> But retransmits_timed_out() function still uses TCP_RTO_MIN and >> tcp_retries{1,2} sysctls don't work properly for tuned socket values. >> >> Fixes: ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") >> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru> >> Acked-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru> >> --- >> net/ipv4/tcp_timer.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c >> index 20cf4a9..66c4b97 100644 >> --- a/net/ipv4/tcp_timer.c >> +++ b/net/ipv4/tcp_timer.c >> @@ -199,12 +199,13 @@ static unsigned int tcp_model_timeout(struct sock *sk, >> * @boundary: max number of retransmissions >> * @timeout: A custom timeout value. >> * If set to 0 the default timeout is calculated and used. >> - * Using TCP_RTO_MIN and the number of unsuccessful retransmits. >> + * Using icsk_rto_min value from socket or RTAX_RTO_MIN from route >> + * and the number of unsuccessful retransmits. >> * >> * The default "timeout" value this function can calculate and use >> * is equivalent to the timeout of a TCP Connection >> * after "boundary" unsuccessful, exponentially backed-off >> - * retransmissions with an initial RTO of TCP_RTO_MIN. >> + * retransmissions with an initial RTO of icsk_rto_min or RTAX_RTO_MIN. >> */ >> static bool retransmits_timed_out(struct sock *sk, >> unsigned int boundary, >> @@ -217,7 +218,7 @@ static bool retransmits_timed_out(struct sock *sk, >> >> start_ts = tcp_sk(sk)->retrans_stamp; >> if (likely(timeout == 0)) { >> - unsigned int rto_base = TCP_RTO_MIN; >> + unsigned int rto_base = tcp_rto_min(sk); >> >> if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) >> rto_base = tcp_timeout_init(sk); >> -- > > I would argue strenuously against this. We tried the approach in this > patch at Google years ago, but we had to revert that approach and go > back to using TCP_RTO_MIN as the baseline for the computation, because > using the custom tcp_rto_min(sk) caused serious reliability problems. > > The behavior in this patch causes various serious reliability problems > because the retransmits_timed_out() computation is used for various > timeout decisions that determine how long a connection tries to > retransmit something before deciding the path is bad and/or giving up > and closing the connection. Here are a few of the problems this > causes: > > (1) The biggest one is probably orphan retries. By default > tcp_orphan_retries() uses a retry count of 8. But if your min_rto is > 5ms (used at Google for many years), then the 8 retries means an > orphaned connection (whose fd is no longer held by a process, but is > still established) only lasts for 1.275 seconds before giving up and > closing. This means that connectivity problems longer than 1.275 > seconds (extremely common with cellular links) are not tolerated for > such connections; the connections often do not receive the data they > were supposed to receive. > > (2) TCP_RETR1 /sysctl_tcp_retries1, used for __dst_negative_advice(), > also has big problems. Even with a min_rto as big as 20ms, on a route > with 150ms RTT, the approach in this patch will cause > retransmits_timed_out() to return true upon the 1st RTO timer firing, > even though TCP_RETR1 is 3. > > (3) TCP_RETR2 /sysctl_tcp_retries2, with a default of 15, used for > regular connection retry lifetimes, also has a massive decrease in > robustness, due to falling from 109 minutes with a 200ms RTO, to > about 2.7 minutes with a min_rto of 5ms. > > neal
On Fri, Jul 30, 2021 at 8:37 AM Dmitry Yakunin <zeil@yandex-team.ru> wrote: > > Hello, Neal! > > Thanks for your reply and explanations. > > I agree with all your points, about safe defaults for both timeouts > and the number of retries. But what the patch does is not changing the > defaults, it only provides a way to work with these values through > bpf, which is important in an environment that is way different from > cellular networks. For example in the modern DC the rto_min value > should correspond with real RTT, that definitely not 200ms. It seems your patch and your analysis are conflating several different issues: (1) how long should rto_min be in datacenter environments? (2) for reliability/robustness, how long should TCP retry to transmit data before giving up? (2) should rto_min just correspond to the real RTT, or other factors (like delayed ACK timers)? I am talking about the reliability/robustness cost of your proposal to tie custom reductions in (1) to automatic custom reductions in (2). (I'm not talking about safe defaults.) If BPF or routing table entries customize rto_min, then it's great for the rto_min knob to customize the RTO timer value to use a lower value in datacenters to speed up loss recovery (1) (as already happens). But just because you customize (1) does not imply that it is safe to massively reduce the answer to (2): it is not safe to cripple reliability/robustness by (as in your proposed patch) having the rto_min setting massively reduce the length of time that a TCP connection retries sending data before giving up and closing the connection. The problem caused by your proposal to have rto_min shorten the retry duration (e.g. a 5ms rto_min leading to only 1.275 seconds of retries) is a general problem of reliability/robustness, not specific to cellular paths. My point about cellular networks was just the most crisp example I could think of, to try to provide a clear and concrete example. If you really think it's important for TCP connections to only retry sending data for 1.275 seconds, then can you please give an example of when this is important, and then please implement a separate customization mechanism for that, rather than forcing all Linux users of the rto_min mechanism to suffer the fallout from tying (1) to (2)? best regards, neal
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index 20cf4a9..66c4b97 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -199,12 +199,13 @@ static unsigned int tcp_model_timeout(struct sock *sk, * @boundary: max number of retransmissions * @timeout: A custom timeout value. * If set to 0 the default timeout is calculated and used. - * Using TCP_RTO_MIN and the number of unsuccessful retransmits. + * Using icsk_rto_min value from socket or RTAX_RTO_MIN from route + * and the number of unsuccessful retransmits. * * The default "timeout" value this function can calculate and use * is equivalent to the timeout of a TCP Connection * after "boundary" unsuccessful, exponentially backed-off - * retransmissions with an initial RTO of TCP_RTO_MIN. + * retransmissions with an initial RTO of icsk_rto_min or RTAX_RTO_MIN. */ static bool retransmits_timed_out(struct sock *sk, unsigned int boundary, @@ -217,7 +218,7 @@ static bool retransmits_timed_out(struct sock *sk, start_ts = tcp_sk(sk)->retrans_stamp; if (likely(timeout == 0)) { - unsigned int rto_base = TCP_RTO_MIN; + unsigned int rto_base = tcp_rto_min(sk); if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) rto_base = tcp_timeout_init(sk);
Commit ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") adds ability to set rto_min value on socket less then default TCP_RTO_MIN. But retransmits_timed_out() function still uses TCP_RTO_MIN and tcp_retries{1,2} sysctls don't work properly for tuned socket values. Fixes: ca584ba07086 ("tcp: bpf: Add TCP_BPF_RTO_MIN for bpf_setsockopt") Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru> Acked-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru> --- net/ipv4/tcp_timer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)