* [PATCH v2 0/2] Fix reconnection latency caused by FIN/ACK handling race @ 2020-02-01 7:18 sj38.park 2020-02-01 7:18 ` [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received sj38.park 2020-02-01 7:18 ` [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test sj38.park 0 siblings, 2 replies; 9+ messages in thread From: sj38.park @ 2020-02-01 7:18 UTC (permalink / raw) To: eric.dumazet, edumazet Cc: davem, aams, ncardwell, netdev, linux-kselftest, linux-kernel, shuah, ycheng, David.Laight, sj38.park, SeongJae Park From: SeongJae Park <sjpark@amazon.de> When closing a connection, the two acks that required to change closing socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in reverse order. This is possible in RSS disabled environments such as a connection inside a host. For example, expected state transitions and required packets for the disconnection will be similar to below flow. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 <--ACK--- 07 FIN_WAIT_2 08 <--FIN/ACK--- 09 TIME_WAIT 10 ---ACK--> 11 LAST_ACK 12 CLOSED CLOSED In some cases such as LINGER option applied socket, the FIN and FIN/ACK will be substituted to RST and RST/ACK, but there is no difference in the main logic. The acks in lines 6 and 8 are the acks. If the line 8 packet is processed before the line 6 packet, it will be just ignored as it is not a expected packet, and the later process of the line 6 packet will change the status of Process A to FIN_WAIT_2, but as it has already handled line 8 packet, it will not go to TIME_WAIT and thus will not send the line 10 packet to Process B. Thus, Process B will left in CLOSE_WAIT status, as below. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 (<--ACK---) 07 (<--FIN/ACK---) 08 (fired in right order) 09 <--FIN/ACK--- 10 <--ACK--- 11 (processed in reverse order) 12 FIN_WAIT_2 Later, if the Process B sends SYN to Process A for reconnection using the same port, Process A will responds with an ACK for the last flow, which has no increased sequence number. Thus, Process A will send RST, wait for TIMEOUT_INIT (one second in default), and then try reconnection. If reconnections are frequent, the one second latency spikes can be a big problem. Below is a tcpdump results of the problem: 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 /* ONE SECOND DELAY */ 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 Patchset Organization --------------------- The first patch fixes the problem by adjusting the first resend delay of the SYN in the case. The second one adds a user space test to reproduce this problem. The patches are based on the v5.5. You can also clone the complete git tree: $ git clone git://github.com/sjp38/linux -b patches/finack_lat/v2 The web is also available: https://github.com/sjp38/linux/tree/patches/finack_lat/v2 Patchset History ---------------- From v1 (https://lore.kernel.org/linux-kselftest/20200131122421.23286-1-sjpark@amazon.com/) - Drop the trivial comment fix patch (Eric Dumazet) - Limit the delay adjustment to only the first SYN resend (Eric Dumazet) - selftest: Avoid use of hard-coded port number (Eric Dumazet) - Explain RST/ACK and FIN/ACK has no big difference (Neal Cardwell) SeongJae Park (2): tcp: Reduce SYN resend delay if a suspicous ACK is received selftests: net: Add FIN_ACK processing order related latency spike test net/ipv4/tcp_input.c | 8 +- tools/testing/selftests/net/.gitignore | 2 + tools/testing/selftests/net/Makefile | 2 + tools/testing/selftests/net/fin_ack_lat.c | 151 +++++++++++++++++++++ tools/testing/selftests/net/fin_ack_lat.sh | 35 +++++ 5 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/net/fin_ack_lat.c create mode 100755 tools/testing/selftests/net/fin_ack_lat.sh -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 7:18 [PATCH v2 0/2] Fix reconnection latency caused by FIN/ACK handling race sj38.park @ 2020-02-01 7:18 ` sj38.park 2020-02-01 13:51 ` Neal Cardwell 2020-02-01 7:18 ` [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test sj38.park 1 sibling, 1 reply; 9+ messages in thread From: sj38.park @ 2020-02-01 7:18 UTC (permalink / raw) To: eric.dumazet, edumazet Cc: davem, aams, ncardwell, netdev, linux-kselftest, linux-kernel, shuah, ycheng, David.Laight, sj38.park, SeongJae Park From: SeongJae Park <sjpark@amazon.de> When closing a connection, the two acks that required to change closing socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in reverse order. This is possible in RSS disabled environments such as a connection inside a host. For example, expected state transitions and required packets for the disconnection will be similar to below flow. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 <--ACK--- 07 FIN_WAIT_2 08 <--FIN/ACK--- 09 TIME_WAIT 10 ---ACK--> 11 LAST_ACK 12 CLOSED CLOSED In some cases such as LINGER option applied socket, the FIN and FIN/ACK will be substituted to RST and RST/ACK, but there is no difference in the main logic. The acks in lines 6 and 8 are the acks. If the line 8 packet is processed before the line 6 packet, it will be just ignored as it is not a expected packet, and the later process of the line 6 packet will change the status of Process A to FIN_WAIT_2, but as it has already handled line 8 packet, it will not go to TIME_WAIT and thus will not send the line 10 packet to Process B. Thus, Process B will left in CLOSE_WAIT status, as below. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 (<--ACK---) 07 (<--FIN/ACK---) 08 (fired in right order) 09 <--FIN/ACK--- 10 <--ACK--- 11 (processed in reverse order) 12 FIN_WAIT_2 Later, if the Process B sends SYN to Process A for reconnection using the same port, Process A will responds with an ACK for the last flow, which has no increased sequence number. Thus, Process A will send RST, wait for TIMEOUT_INIT (one second in default), and then try reconnection. If reconnections are frequent, the one second latency spikes can be a big problem. Below is a tcpdump results of the problem: 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 /* ONE SECOND DELAY */ 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 This commit mitigates the problem by reducing the delay for the next SYN if the suspicous ACK is received while in SYN_SENT state. Following commit will add a selftest, which can be also helpful for understanding of this issue. Signed-off-by: SeongJae Park <sjpark@amazon.de> --- net/ipv4/tcp_input.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2a976f57f7e7..980bd04b9d95 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, * the segment and return)" */ if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { + /* Previous FIN/ACK or RST/ACK might be ignored. */ + if (icsk->icsk_retransmits == 0) + inet_csk_reset_xmit_timer(sk, + ICSK_TIME_RETRANS, TCP_ATO_MIN, + TCP_RTO_MAX); goto reset_and_undo; + } if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 7:18 ` [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received sj38.park @ 2020-02-01 13:51 ` Neal Cardwell 2020-02-01 14:36 ` SeongJae Park 0 siblings, 1 reply; 9+ messages in thread From: Neal Cardwell @ 2020-02-01 13:51 UTC (permalink / raw) To: sj38.park Cc: Eric Dumazet, Eric Dumazet, David Miller, aams, Netdev, linux-kselftest, LKML, shuah, Yuchung Cheng, David Laight, SeongJae Park On Sat, Feb 1, 2020 at 2:19 AM <sj38.park@gmail.com> wrote: > > From: SeongJae Park <sjpark@amazon.de> > > When closing a connection, the two acks that required to change closing > socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in > reverse order. This is possible in RSS disabled environments such as a > connection inside a host. > > For example, expected state transitions and required packets for the > disconnection will be similar to below flow. > > 00 (Process A) (Process B) > 01 ESTABLISHED ESTABLISHED > 02 close() > 03 FIN_WAIT_1 > 04 ---FIN--> > 05 CLOSE_WAIT > 06 <--ACK--- > 07 FIN_WAIT_2 > 08 <--FIN/ACK--- > 09 TIME_WAIT > 10 ---ACK--> > 11 LAST_ACK > 12 CLOSED CLOSED > > In some cases such as LINGER option applied socket, the FIN and FIN/ACK > will be substituted to RST and RST/ACK, but there is no difference in > the main logic. > > The acks in lines 6 and 8 are the acks. If the line 8 packet is > processed before the line 6 packet, it will be just ignored as it is not > a expected packet, and the later process of the line 6 packet will > change the status of Process A to FIN_WAIT_2, but as it has already > handled line 8 packet, it will not go to TIME_WAIT and thus will not > send the line 10 packet to Process B. Thus, Process B will left in > CLOSE_WAIT status, as below. > > 00 (Process A) (Process B) > 01 ESTABLISHED ESTABLISHED > 02 close() > 03 FIN_WAIT_1 > 04 ---FIN--> > 05 CLOSE_WAIT > 06 (<--ACK---) > 07 (<--FIN/ACK---) > 08 (fired in right order) > 09 <--FIN/ACK--- > 10 <--ACK--- > 11 (processed in reverse order) > 12 FIN_WAIT_2 > > Later, if the Process B sends SYN to Process A for reconnection using > the same port, Process A will responds with an ACK for the last flow, > which has no increased sequence number. Thus, Process A will send RST, > wait for TIMEOUT_INIT (one second in default), and then try > reconnection. If reconnections are frequent, the one second latency > spikes can be a big problem. Below is a tcpdump results of the problem: > > 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 > 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 > /* ONE SECOND DELAY */ > 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > This commit mitigates the problem by reducing the delay for the next SYN > if the suspicous ACK is received while in SYN_SENT state. > > Following commit will add a selftest, which can be also helpful for > understanding of this issue. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > --- > net/ipv4/tcp_input.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 2a976f57f7e7..980bd04b9d95 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, > * the segment and return)" > */ > if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || > - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) > + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { > + /* Previous FIN/ACK or RST/ACK might be ignored. */ > + if (icsk->icsk_retransmits == 0) > + inet_csk_reset_xmit_timer(sk, > + ICSK_TIME_RETRANS, TCP_ATO_MIN, > + TCP_RTO_MAX); > goto reset_and_undo; > + } > > if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && > !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, > -- Scheduling a timer for TCP_ATO_MIN, typically 40ms, sounds like it might be a bit on the slow side. How about TCP_TIMEOUT_MIN, which is typically 2ms on a HZ=1000 kernel? I think this would be closer to what Eric mentioned: "sending the SYN a few ms after the RST seems way better than waiting 1 second as if we received no packet at all." neal ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 13:51 ` Neal Cardwell @ 2020-02-01 14:36 ` SeongJae Park 2020-02-01 14:53 ` [PATCH v2.1 " sj38.park 0 siblings, 1 reply; 9+ messages in thread From: SeongJae Park @ 2020-02-01 14:36 UTC (permalink / raw) To: Neal Cardwell Cc: sj38.park, Eric Dumazet, Eric Dumazet, David Miller, aams, Netdev, linux-kselftest, LKML, shuah, Yuchung Cheng, David Laight, SeongJae Park On Sat, 1 Feb 2020 08:51:48 -0500 Neal Cardwell <ncardwell@google.com> wrote: > On Sat, Feb 1, 2020 at 2:19 AM <sj38.park@gmail.com> wrote: > > > > From: SeongJae Park <sjpark@amazon.de> > > > > When closing a connection, the two acks that required to change closing > > socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in > > reverse order. This is possible in RSS disabled environments such as a > > connection inside a host. > > > > For example, expected state transitions and required packets for the > > disconnection will be similar to below flow. > > > > 00 (Process A) (Process B) > > 01 ESTABLISHED ESTABLISHED > > 02 close() > > 03 FIN_WAIT_1 > > 04 ---FIN--> > > 05 CLOSE_WAIT > > 06 <--ACK--- > > 07 FIN_WAIT_2 > > 08 <--FIN/ACK--- > > 09 TIME_WAIT > > 10 ---ACK--> > > 11 LAST_ACK > > 12 CLOSED CLOSED > > > > In some cases such as LINGER option applied socket, the FIN and FIN/ACK > > will be substituted to RST and RST/ACK, but there is no difference in > > the main logic. > > > > The acks in lines 6 and 8 are the acks. If the line 8 packet is > > processed before the line 6 packet, it will be just ignored as it is not > > a expected packet, and the later process of the line 6 packet will > > change the status of Process A to FIN_WAIT_2, but as it has already > > handled line 8 packet, it will not go to TIME_WAIT and thus will not > > send the line 10 packet to Process B. Thus, Process B will left in > > CLOSE_WAIT status, as below. > > > > 00 (Process A) (Process B) > > 01 ESTABLISHED ESTABLISHED > > 02 close() > > 03 FIN_WAIT_1 > > 04 ---FIN--> > > 05 CLOSE_WAIT > > 06 (<--ACK---) > > 07 (<--FIN/ACK---) > > 08 (fired in right order) > > 09 <--FIN/ACK--- > > 10 <--ACK--- > > 11 (processed in reverse order) > > 12 FIN_WAIT_2 > > > > Later, if the Process B sends SYN to Process A for reconnection using > > the same port, Process A will responds with an ACK for the last flow, > > which has no increased sequence number. Thus, Process A will send RST, > > wait for TIMEOUT_INIT (one second in default), and then try > > reconnection. If reconnections are frequent, the one second latency > > spikes can be a big problem. Below is a tcpdump results of the problem: > > > > 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 > > 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 > > /* ONE SECOND DELAY */ > > 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > > > This commit mitigates the problem by reducing the delay for the next SYN > > if the suspicous ACK is received while in SYN_SENT state. > > > > Following commit will add a selftest, which can be also helpful for > > understanding of this issue. > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > --- > > net/ipv4/tcp_input.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > index 2a976f57f7e7..980bd04b9d95 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, > > * the segment and return)" > > */ > > if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || > > - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) > > + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { > > + /* Previous FIN/ACK or RST/ACK might be ignored. */ > > + if (icsk->icsk_retransmits == 0) > > + inet_csk_reset_xmit_timer(sk, > > + ICSK_TIME_RETRANS, TCP_ATO_MIN, > > + TCP_RTO_MAX); > > goto reset_and_undo; > > + } > > > > if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && > > !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, > > -- > > Scheduling a timer for TCP_ATO_MIN, typically 40ms, sounds like it > might be a bit on the slow side. How about TCP_TIMEOUT_MIN, which is > typically 2ms on a HZ=1000 kernel? > > I think this would be closer to what Eric mentioned: "sending the SYN > a few ms after the RST seems way better than waiting 1 second as if we > received no packet at all." Agreed, it seems much better! Because this is just a small change in a tiny patchset containing only two patches, I will send the updated version of only this patch in reply to this mail, as soon as I finish tests. Thanks, SeongJae Park > > neal ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2.1 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 14:36 ` SeongJae Park @ 2020-02-01 14:53 ` sj38.park 2020-02-01 18:23 ` Eric Dumazet 0 siblings, 1 reply; 9+ messages in thread From: sj38.park @ 2020-02-01 14:53 UTC (permalink / raw) To: sj38.park Cc: David.Laight, aams, davem, edumazet, eric.dumazet, linux-kernel, linux-kselftest, ncardwell, netdev, shuah, sjpark From: SeongJae Park <sjpark@amazon.de> When closing a connection, the two acks that required to change closing socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in reverse order. This is possible in RSS disabled environments such as a connection inside a host. For example, expected state transitions and required packets for the disconnection will be similar to below flow. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 <--ACK--- 07 FIN_WAIT_2 08 <--FIN/ACK--- 09 TIME_WAIT 10 ---ACK--> 11 LAST_ACK 12 CLOSED CLOSED In some cases such as LINGER option applied socket, the FIN and FIN/ACK will be substituted to RST and RST/ACK, but there is no difference in the main logic. The acks in lines 6 and 8 are the acks. If the line 8 packet is processed before the line 6 packet, it will be just ignored as it is not a expected packet, and the later process of the line 6 packet will change the status of Process A to FIN_WAIT_2, but as it has already handled line 8 packet, it will not go to TIME_WAIT and thus will not send the line 10 packet to Process B. Thus, Process B will left in CLOSE_WAIT status, as below. 00 (Process A) (Process B) 01 ESTABLISHED ESTABLISHED 02 close() 03 FIN_WAIT_1 04 ---FIN--> 05 CLOSE_WAIT 06 (<--ACK---) 07 (<--FIN/ACK---) 08 (fired in right order) 09 <--FIN/ACK--- 10 <--ACK--- 11 (processed in reverse order) 12 FIN_WAIT_2 Later, if the Process B sends SYN to Process A for reconnection using the same port, Process A will responds with an ACK for the last flow, which has no increased sequence number. Thus, Process A will send RST, wait for TIMEOUT_INIT (one second in default), and then try reconnection. If reconnections are frequent, the one second latency spikes can be a big problem. Below is a tcpdump results of the problem: 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 /* ONE SECOND DELAY */ 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 This commit mitigates the problem by reducing the delay for the next SYN if the suspicous ACK is received while in SYN_SENT state. Following commit will add a selftest, which can be also helpful for understanding of this issue. Signed-off-by: SeongJae Park <sjpark@amazon.de> --- net/ipv4/tcp_input.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2a976f57f7e7..baa4fee117f9 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, * the segment and return)" */ if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { + /* Previous FIN/ACK or RST/ACK might be ignored. */ + if (icsk->icsk_retransmits == 0) + inet_csk_reset_xmit_timer(sk, + ICSK_TIME_RETRANS, + TCP_TIMEOUT_MIN, TCP_RTO_MAX); goto reset_and_undo; + } if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2.1 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 14:53 ` [PATCH v2.1 " sj38.park @ 2020-02-01 18:23 ` Eric Dumazet 2020-02-02 3:40 ` SeongJae Park 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2020-02-01 18:23 UTC (permalink / raw) To: sj38.park Cc: David.Laight, aams, davem, edumazet, linux-kernel, linux-kselftest, ncardwell, netdev, shuah, sjpark On 2/1/20 6:53 AM, sj38.park@gmail.com wrote: > From: SeongJae Park <sjpark@amazon.de> > > When closing a connection, the two acks that required to change closing > socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in > reverse order. This is possible in RSS disabled environments such as a > connection inside a host. > > For example, expected state transitions and required packets for the > disconnection will be similar to below flow. > > 00 (Process A) (Process B) > 01 ESTABLISHED ESTABLISHED > 02 close() > 03 FIN_WAIT_1 > 04 ---FIN--> > 05 CLOSE_WAIT > 06 <--ACK--- > 07 FIN_WAIT_2 > 08 <--FIN/ACK--- > 09 TIME_WAIT > 10 ---ACK--> > 11 LAST_ACK > 12 CLOSED CLOSED > > In some cases such as LINGER option applied socket, the FIN and FIN/ACK > will be substituted to RST and RST/ACK, but there is no difference in > the main logic. > > The acks in lines 6 and 8 are the acks. If the line 8 packet is > processed before the line 6 packet, it will be just ignored as it is not > a expected packet, and the later process of the line 6 packet will > change the status of Process A to FIN_WAIT_2, but as it has already > handled line 8 packet, it will not go to TIME_WAIT and thus will not > send the line 10 packet to Process B. Thus, Process B will left in > CLOSE_WAIT status, as below. > > 00 (Process A) (Process B) > 01 ESTABLISHED ESTABLISHED > 02 close() > 03 FIN_WAIT_1 > 04 ---FIN--> > 05 CLOSE_WAIT > 06 (<--ACK---) > 07 (<--FIN/ACK---) > 08 (fired in right order) > 09 <--FIN/ACK--- > 10 <--ACK--- > 11 (processed in reverse order) > 12 FIN_WAIT_2 > > Later, if the Process B sends SYN to Process A for reconnection using > the same port, Process A will responds with an ACK for the last flow, > which has no increased sequence number. Thus, Process A will send RST, > wait for TIMEOUT_INIT (one second in default), and then try > reconnection. If reconnections are frequent, the one second latency > spikes can be a big problem. Below is a tcpdump results of the problem: > > 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 > 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 > /* ONE SECOND DELAY */ > 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > This commit mitigates the problem by reducing the delay for the next SYN > if the suspicous ACK is received while in SYN_SENT state. > > Following commit will add a selftest, which can be also helpful for > understanding of this issue. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > --- > net/ipv4/tcp_input.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 2a976f57f7e7..baa4fee117f9 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, > * the segment and return)" > */ > if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || > - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) > + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { > + /* Previous FIN/ACK or RST/ACK might be ignored. */ > + if (icsk->icsk_retransmits == 0) > + inet_csk_reset_xmit_timer(sk, > + ICSK_TIME_RETRANS, > + TCP_TIMEOUT_MIN, TCP_RTO_MAX); > goto reset_and_undo; > + } > > if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && > !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, > Please add my Signed-off-by: Eric Dumazet <edumazet@google.com> Please resend the whole patch series as requested by netdev maintainers. vi +134 Documentation/networking/netdev-FAQ.rst Q: I made changes to only a few patches in a patch series should I resend only those changed? --------------------------------------------------------------------------------------------- A: No, please resend the entire patch series and make sure you do number your patches such that it is clear this is the latest and greatest set of patches that can be applied. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v2.1 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received 2020-02-01 18:23 ` Eric Dumazet @ 2020-02-02 3:40 ` SeongJae Park 0 siblings, 0 replies; 9+ messages in thread From: SeongJae Park @ 2020-02-02 3:40 UTC (permalink / raw) To: Eric Dumazet Cc: sj38.park, David.Laight, aams, davem, edumazet, linux-kernel, linux-kselftest, ncardwell, netdev, shuah, sjpark On Sat, 1 Feb 2020 10:23:43 -0800 Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On 2/1/20 6:53 AM, sj38.park@gmail.com wrote: > > From: SeongJae Park <sjpark@amazon.de> > > > > When closing a connection, the two acks that required to change closing > > socket's status to FIN_WAIT_2 and then TIME_WAIT could be processed in > > reverse order. This is possible in RSS disabled environments such as a > > connection inside a host. > > > > For example, expected state transitions and required packets for the > > disconnection will be similar to below flow. > > > > 00 (Process A) (Process B) > > 01 ESTABLISHED ESTABLISHED > > 02 close() > > 03 FIN_WAIT_1 > > 04 ---FIN--> > > 05 CLOSE_WAIT > > 06 <--ACK--- > > 07 FIN_WAIT_2 > > 08 <--FIN/ACK--- > > 09 TIME_WAIT > > 10 ---ACK--> > > 11 LAST_ACK > > 12 CLOSED CLOSED > > > > In some cases such as LINGER option applied socket, the FIN and FIN/ACK > > will be substituted to RST and RST/ACK, but there is no difference in > > the main logic. > > > > The acks in lines 6 and 8 are the acks. If the line 8 packet is > > processed before the line 6 packet, it will be just ignored as it is not > > a expected packet, and the later process of the line 6 packet will > > change the status of Process A to FIN_WAIT_2, but as it has already > > handled line 8 packet, it will not go to TIME_WAIT and thus will not > > send the line 10 packet to Process B. Thus, Process B will left in > > CLOSE_WAIT status, as below. > > > > 00 (Process A) (Process B) > > 01 ESTABLISHED ESTABLISHED > > 02 close() > > 03 FIN_WAIT_1 > > 04 ---FIN--> > > 05 CLOSE_WAIT > > 06 (<--ACK---) > > 07 (<--FIN/ACK---) > > 08 (fired in right order) > > 09 <--FIN/ACK--- > > 10 <--ACK--- > > 11 (processed in reverse order) > > 12 FIN_WAIT_2 > > > > Later, if the Process B sends SYN to Process A for reconnection using > > the same port, Process A will responds with an ACK for the last flow, > > which has no increased sequence number. Thus, Process A will send RST, > > wait for TIMEOUT_INIT (one second in default), and then try > > reconnection. If reconnections are frequent, the one second latency > > spikes can be a big problem. Below is a tcpdump results of the problem: > > > > 14.436259 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > 14.436266 IP 127.0.0.1.4242 > 127.0.0.1.45150: Flags [.], ack 5, win 512 > > 14.436271 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [R], seq 2541101298 > > /* ONE SECOND DELAY */ > > 15.464613 IP 127.0.0.1.45150 > 127.0.0.1.4242: Flags [S], seq 2560603644 > > > > This commit mitigates the problem by reducing the delay for the next SYN > > if the suspicous ACK is received while in SYN_SENT state. > > > > Following commit will add a selftest, which can be also helpful for > > understanding of this issue. > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > --- > > net/ipv4/tcp_input.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > index 2a976f57f7e7..baa4fee117f9 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -5893,8 +5893,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb, > > * the segment and return)" > > */ > > if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) || > > - after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) > > + after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { > > + /* Previous FIN/ACK or RST/ACK might be ignored. */ > > + if (icsk->icsk_retransmits == 0) > > + inet_csk_reset_xmit_timer(sk, > > + ICSK_TIME_RETRANS, > > + TCP_TIMEOUT_MIN, TCP_RTO_MAX); > > goto reset_and_undo; > > + } > > > > if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr && > > !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp, > > > > Please add my > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Please resend the whole patch series as requested by netdev maintainers. > > > vi +134 Documentation/networking/netdev-FAQ.rst > > Q: I made changes to only a few patches in a patch series should I resend only those changed? > --------------------------------------------------------------------------------------------- > A: No, please resend the entire patch series and make sure you do number your > patches such that it is clear this is the latest and greatest set of patches > that can be applied. Thank you, just sent it: https://lore.kernel.org/linux-kselftest/20200202033827.16304-1-sj38.park@gmail.com/ Also, appreciate for kindly noticing the rule :) Thanks, SeongJae Park > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test 2020-02-01 7:18 [PATCH v2 0/2] Fix reconnection latency caused by FIN/ACK handling race sj38.park 2020-02-01 7:18 ` [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received sj38.park @ 2020-02-01 7:18 ` sj38.park 2020-02-01 18:20 ` Eric Dumazet 1 sibling, 1 reply; 9+ messages in thread From: sj38.park @ 2020-02-01 7:18 UTC (permalink / raw) To: eric.dumazet, edumazet Cc: davem, aams, ncardwell, netdev, linux-kselftest, linux-kernel, shuah, ycheng, David.Laight, sj38.park, SeongJae Park From: SeongJae Park <sjpark@amazon.de> This commit adds a test for FIN_ACK process races related reconnection latency spike issues. The issue has described and solved by the previous commit ("tcp: Reduce SYN resend delay if a suspicous ACK is received"). The test program is configured with a server and a client process. The server creates and binds a socket to a port that dynamically allocated, listen on it, and start a infinite loop. Inside the loop, it accepts connection, reads 4 bytes from the socket, and closes the connection. The client is constructed as an infinite loop. Inside the loop, it creates a socket with LINGER and NODELAY option, connect to the server, send 4 bytes data, try read some data from server. After the read() returns, it measure the latency from the beginning of this loop to this point and if the latency is larger than 1 second (spike), print a message. Signed-off-by: SeongJae Park <sjpark@amazon.de> --- tools/testing/selftests/net/.gitignore | 2 + tools/testing/selftests/net/Makefile | 2 + tools/testing/selftests/net/fin_ack_lat.c | 151 +++++++++++++++++++++ tools/testing/selftests/net/fin_ack_lat.sh | 35 +++++ 4 files changed, 190 insertions(+) create mode 100644 tools/testing/selftests/net/fin_ack_lat.c create mode 100755 tools/testing/selftests/net/fin_ack_lat.sh diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore index 8aefd81fbc86..1bcf7b5498dd 100644 --- a/tools/testing/selftests/net/.gitignore +++ b/tools/testing/selftests/net/.gitignore @@ -22,3 +22,5 @@ ipv6_flowlabel_mgr so_txtime tcp_fastopen_backup_key nettest +fin_ack_lat_accept +fin_ack_lat_connect diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index a8e04d665b69..b5694196430a 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -11,6 +11,7 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh traceroute.sh +TEST_PROGS += fin_ack_lat.sh TEST_PROGS_EXTENDED := in_netns.sh TEST_GEN_FILES = socket nettest TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any @@ -18,6 +19,7 @@ TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr TEST_GEN_FILES += tcp_fastopen_backup_key +TEST_GEN_FILES += fin_ack_lat TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls diff --git a/tools/testing/selftests/net/fin_ack_lat.c b/tools/testing/selftests/net/fin_ack_lat.c new file mode 100644 index 000000000000..70187494b57a --- /dev/null +++ b/tools/testing/selftests/net/fin_ack_lat.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <arpa/inet.h> +#include <errno.h> +#include <error.h> +#include <netinet/in.h> +#include <netinet/tcp.h> +#include <signal.h> +#include <stdio.h> +#include <stdlib.h> +#include <sys/socket.h> +#include <sys/time.h> +#include <unistd.h> + +static int child_pid; + +static unsigned long timediff(struct timeval s, struct timeval e) +{ + unsigned long s_us, e_us; + + s_us = s.tv_sec * 1000000 + s.tv_usec; + e_us = e.tv_sec * 1000000 + e.tv_usec; + if (s_us > e_us) + return 0; + return e_us - s_us; +} + +static void client(int port) +{ + int sock = 0; + struct sockaddr_in addr, laddr; + socklen_t len = sizeof(laddr); + struct linger sl; + int flag = 1; + int buffer; + struct timeval start, end; + unsigned long lat, sum_lat = 0, nr_lat = 0; + + while (1) { + gettimeofday(&start, NULL); + + sock = socket(AF_INET, SOCK_STREAM, 0); + if (sock < 0) + error(-1, errno, "socket creation"); + + sl.l_onoff = 1; + sl.l_linger = 0; + if (setsockopt(sock, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl))) + error(-1, errno, "setsockopt(linger)"); + + if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, + &flag, sizeof(flag))) + error(-1, errno, "setsockopt(nodelay)"); + + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + + if (inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr) <= 0) + error(-1, errno, "inet_pton"); + + if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0) + error(-1, errno, "connect"); + + send(sock, &buffer, sizeof(buffer), 0); + if (read(sock, &buffer, sizeof(buffer)) == -1) + error(-1, errno, "waiting read"); + + gettimeofday(&end, NULL); + lat = timediff(start, end); + sum_lat += lat; + nr_lat++; + if (lat < 100000) + goto close; + + if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1) + error(-1, errno, "getsockname"); + printf("port: %d, lat: %lu, avg: %lu, nr: %lu\n", + ntohs(laddr.sin_port), lat, + sum_lat / nr_lat, nr_lat); +close: + fflush(stdout); + close(sock); + } +} + +static void server(int sock, struct sockaddr_in address) +{ + int accepted; + int addrlen = sizeof(address); + int buffer; + + while (1) { + accepted = accept(sock, (struct sockaddr *)&address, + (socklen_t *)&addrlen); + if (accepted < 0) + error(-1, errno, "accept"); + + if (read(accepted, &buffer, sizeof(buffer)) == -1) + error(-1, errno, "read"); + close(accepted); + } +} + +static void sig_handler(int signum) +{ + kill(SIGTERM, child_pid); + exit(0); +} + +int main(int argc, char const *argv[]) +{ + int sock; + int opt = 1; + struct sockaddr_in address; + struct sockaddr_in laddr; + socklen_t len = sizeof(laddr); + + if (signal(SIGTERM, sig_handler) == SIG_ERR) + error(-1, errno, "signal"); + + sock = socket(AF_INET, SOCK_STREAM, 0); + if (sock < 0) + error(-1, errno, "socket"); + + if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR | SO_REUSEPORT, + &opt, sizeof(opt)) == -1) + error(-1, errno, "setsockopt"); + + address.sin_family = AF_INET; + address.sin_addr.s_addr = INADDR_ANY; + /* dynamically allocate unused port */ + address.sin_port = 0; + + if (bind(sock, (struct sockaddr *)&address, sizeof(address)) < 0) + error(-1, errno, "bind"); + + if (listen(sock, 3) < 0) + error(-1, errno, "listen"); + + if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1) + error(-1, errno, "getsockname"); + + fprintf(stderr, "server port: %d\n", ntohs(laddr.sin_port)); + child_pid = fork(); + if (!child_pid) + client(ntohs(laddr.sin_port)); + else + server(sock, laddr); + + return 0; +} diff --git a/tools/testing/selftests/net/fin_ack_lat.sh b/tools/testing/selftests/net/fin_ack_lat.sh new file mode 100755 index 000000000000..a3ff6e0b2c7a --- /dev/null +++ b/tools/testing/selftests/net/fin_ack_lat.sh @@ -0,0 +1,35 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Test latency spikes caused by FIN/ACK handling race. + +set +x +set -e + +tmpfile=$(mktemp /tmp/fin_ack_latency.XXXX.log) + +cleanup() { + kill $(pidof fin_ack_lat) + rm -f $tmpfile +} + +trap cleanup EXIT + +do_test() { + RUNTIME=$1 + + ./fin_ack_lat | tee $tmpfile & + PID=$! + + sleep $RUNTIME + NR_SPIKES=$(wc -l $tmpfile | awk '{print $1}') + if [ $NR_SPIKES -gt 0 ] + then + echo "FAIL: $NR_SPIKES spikes detected" + return 1 + fi + return 0 +} + +do_test "30" +echo "test done" -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test 2020-02-01 7:18 ` [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test sj38.park @ 2020-02-01 18:20 ` Eric Dumazet 0 siblings, 0 replies; 9+ messages in thread From: Eric Dumazet @ 2020-02-01 18:20 UTC (permalink / raw) To: sj38.park, edumazet Cc: davem, aams, ncardwell, netdev, linux-kselftest, linux-kernel, shuah, ycheng, David.Laight, SeongJae Park On 1/31/20 11:18 PM, sj38.park@gmail.com wrote: > From: SeongJae Park <sjpark@amazon.de> > > This commit adds a test for FIN_ACK process races related reconnection > latency spike issues. The issue has described and solved by the > previous commit ("tcp: Reduce SYN resend delay if a suspicous ACK is > received"). > > The test program is configured with a server and a client process. The > server creates and binds a socket to a port that dynamically allocated, > listen on it, and start a infinite loop. Inside the loop, it accepts > connection, reads 4 bytes from the socket, and closes the connection. > The client is constructed as an infinite loop. Inside the loop, it > creates a socket with LINGER and NODELAY option, connect to the server, > send 4 bytes data, try read some data from server. After the read() > returns, it measure the latency from the beginning of this loop to this > point and if the latency is larger than 1 second (spike), print a > message. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> Reviewed-by: Eric Dumazet <edumazet@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-02 3:40 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-01 7:18 [PATCH v2 0/2] Fix reconnection latency caused by FIN/ACK handling race sj38.park 2020-02-01 7:18 ` [PATCH v2 1/2] tcp: Reduce SYN resend delay if a suspicous ACK is received sj38.park 2020-02-01 13:51 ` Neal Cardwell 2020-02-01 14:36 ` SeongJae Park 2020-02-01 14:53 ` [PATCH v2.1 " sj38.park 2020-02-01 18:23 ` Eric Dumazet 2020-02-02 3:40 ` SeongJae Park 2020-02-01 7:18 ` [PATCH v2 2/2] selftests: net: Add FIN_ACK processing order related latency spike test sj38.park 2020-02-01 18:20 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).