Skip to content

Commit 83d0585

Browse files
kuba-mookuba-moo
committed
Merge branch 'Fix-reconnection-latency-caused-by-FIN-ACK-handling-race'
SeongJae Park says: ==================== Fix reconnection latency caused by FIN/ACK handling race 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. From v2 (https://lore.kernel.org/linux-kselftest/[email protected]/) - Use TCP_TIMEOUT_MIN as reduced delay (Neal Cardwall) - Add Reviewed-by and Signed-off-by from Eric Dumazet From v1 (https://lore.kernel.org/linux-kselftest/[email protected]/) - 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) ==================== Signed-off-by: Jakub Kicinski <[email protected]>
2 parents dff6bc1 + af8c8a4 commit 83d0585

File tree

5 files changed

+196
-1
lines changed

5 files changed

+196
-1
lines changed

‎net/ipv4/tcp_input.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5908,8 +5908,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
59085908
* the segment and return)"
59095909
*/
59105910
if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) ||
5911-
after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt))
5911+
after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
5912+
/* Previous FIN/ACK or RST/ACK might be ignored. */
5913+
if (icsk->icsk_retransmits == 0)
5914+
inet_csk_reset_xmit_timer(sk,
5915+
ICSK_TIME_RETRANS,
5916+
TCP_TIMEOUT_MIN, TCP_RTO_MAX);
59125917
goto reset_and_undo;
5918+
}
59135919

59145920
if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr &&
59155921
!between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp,

‎tools/testing/selftests/net/.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,4 @@ ipv6_flowlabel_mgr
2222
so_txtime
2323
tcp_fastopen_backup_key
2424
nettest
25+
fin_ack_lat

‎tools/testing/selftests/net/Makefile

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
1111
TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
1212
TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh
1313
TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh traceroute.sh
14+
TEST_PROGS += fin_ack_lat.sh
1415
TEST_PROGS_EXTENDED := in_netns.sh
1516
TEST_GEN_FILES = socket nettest
1617
TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
1718
TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
1819
TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag
1920
TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr
2021
TEST_GEN_FILES += tcp_fastopen_backup_key
22+
TEST_GEN_FILES += fin_ack_lat
2123
TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
2224
TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
2325

‎tools/testing/selftests/net/fin_ack_lat.c

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
// SPDX-License-Identifier: GPL-2.0
2+
3+
#include <arpa/inet.h>
4+
#include <errno.h>
5+
#include <error.h>
6+
#include <netinet/in.h>
7+
#include <netinet/tcp.h>
8+
#include <signal.h>
9+
#include <stdio.h>
10+
#include <stdlib.h>
11+
#include <sys/socket.h>
12+
#include <sys/time.h>
13+
#include <unistd.h>
14+
15+
static int child_pid;
16+
17+
static unsigned long timediff(struct timeval s, struct timeval e)
18+
{
19+
unsigned long s_us, e_us;
20+
21+
s_us = s.tv_sec * 1000000 + s.tv_usec;
22+
e_us = e.tv_sec * 1000000 + e.tv_usec;
23+
if (s_us > e_us)
24+
return 0;
25+
return e_us - s_us;
26+
}
27+
28+
static void client(int port)
29+
{
30+
int sock = 0;
31+
struct sockaddr_in addr, laddr;
32+
socklen_t len = sizeof(laddr);
33+
struct linger sl;
34+
int flag = 1;
35+
int buffer;
36+
struct timeval start, end;
37+
unsigned long lat, sum_lat = 0, nr_lat = 0;
38+
39+
while (1) {
40+
gettimeofday(&start, NULL);
41+
42+
sock = socket(AF_INET, SOCK_STREAM, 0);
43+
if (sock < 0)
44+
error(-1, errno, "socket creation");
45+
46+
sl.l_onoff = 1;
47+
sl.l_linger = 0;
48+
if (setsockopt(sock, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl)))
49+
error(-1, errno, "setsockopt(linger)");
50+
51+
if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
52+
&flag, sizeof(flag)))
53+
error(-1, errno, "setsockopt(nodelay)");
54+
55+
addr.sin_family = AF_INET;
56+
addr.sin_port = htons(port);
57+
58+
if (inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr) <= 0)
59+
error(-1, errno, "inet_pton");
60+
61+
if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0)
62+
error(-1, errno, "connect");
63+
64+
send(sock, &buffer, sizeof(buffer), 0);
65+
if (read(sock, &buffer, sizeof(buffer)) == -1)
66+
error(-1, errno, "waiting read");
67+
68+
gettimeofday(&end, NULL);
69+
lat = timediff(start, end);
70+
sum_lat += lat;
71+
nr_lat++;
72+
if (lat < 100000)
73+
goto close;
74+
75+
if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1)
76+
error(-1, errno, "getsockname");
77+
printf("port: %d, lat: %lu, avg: %lu, nr: %lu\n",
78+
ntohs(laddr.sin_port), lat,
79+
sum_lat / nr_lat, nr_lat);
80+
close:
81+
fflush(stdout);
82+
close(sock);
83+
}
84+
}
85+
86+
static void server(int sock, struct sockaddr_in address)
87+
{
88+
int accepted;
89+
int addrlen = sizeof(address);
90+
int buffer;
91+
92+
while (1) {
93+
accepted = accept(sock, (struct sockaddr *)&address,
94+
(socklen_t *)&addrlen);
95+
if (accepted < 0)
96+
error(-1, errno, "accept");
97+
98+
if (read(accepted, &buffer, sizeof(buffer)) == -1)
99+
error(-1, errno, "read");
100+
close(accepted);
101+
}
102+
}
103+
104+
static void sig_handler(int signum)
105+
{
106+
kill(SIGTERM, child_pid);
107+
exit(0);
108+
}
109+
110+
int main(int argc, char const *argv[])
111+
{
112+
int sock;
113+
int opt = 1;
114+
struct sockaddr_in address;
115+
struct sockaddr_in laddr;
116+
socklen_t len = sizeof(laddr);
117+
118+
if (signal(SIGTERM, sig_handler) == SIG_ERR)
119+
error(-1, errno, "signal");
120+
121+
sock = socket(AF_INET, SOCK_STREAM, 0);
122+
if (sock < 0)
123+
error(-1, errno, "socket");
124+
125+
if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR | SO_REUSEPORT,
126+
&opt, sizeof(opt)) == -1)
127+
error(-1, errno, "setsockopt");
128+
129+
address.sin_family = AF_INET;
130+
address.sin_addr.s_addr = INADDR_ANY;
131+
/* dynamically allocate unused port */
132+
address.sin_port = 0;
133+
134+
if (bind(sock, (struct sockaddr *)&address, sizeof(address)) < 0)
135+
error(-1, errno, "bind");
136+
137+
if (listen(sock, 3) < 0)
138+
error(-1, errno, "listen");
139+
140+
if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1)
141+
error(-1, errno, "getsockname");
142+
143+
fprintf(stderr, "server port: %d\n", ntohs(laddr.sin_port));
144+
child_pid = fork();
145+
if (!child_pid)
146+
client(ntohs(laddr.sin_port));
147+
else
148+
server(sock, laddr);
149+
150+
return 0;
151+
}

‎tools/testing/selftests/net/fin_ack_lat.sh

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#!/bin/bash
2+
# SPDX-License-Identifier: GPL-2.0
3+
#
4+
# Test latency spikes caused by FIN/ACK handling race.
5+
6+
set +x
7+
set -e
8+
9+
tmpfile=$(mktemp /tmp/fin_ack_latency.XXXX.log)
10+
11+
cleanup() {
12+
kill $(pidof fin_ack_lat)
13+
rm -f $tmpfile
14+
}
15+
16+
trap cleanup EXIT
17+
18+
do_test() {
19+
RUNTIME=$1
20+
21+
./fin_ack_lat | tee $tmpfile &
22+
PID=$!
23+
24+
sleep $RUNTIME
25+
NR_SPIKES=$(wc -l $tmpfile | awk '{print $1}')
26+
if [ $NR_SPIKES -gt 0 ]
27+
then
28+
echo "FAIL: $NR_SPIKES spikes detected"
29+
return 1
30+
fi
31+
return 0
32+
}
33+
34+
do_test "30"
35+
echo "test done"

0 commit comments

Comments
 (0)