| Message ID | 20190816160339.249832-1-ppenkov.kernel@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix race in test_tcp_rtt test | expand |
On 08/16, Petar Penkov wrote: > From: Petar Penkov <ppenkov@google.com> > > There is a race in this test between receiving the ACK for the > single-byte packet sent in the test, and reading the values from the > map. > > This patch fixes this by having the client wait until there are no more > unacknowledged packets. > > Before: > for i in {1..1000}; do ../net/in_netns.sh ./test_tcp_rtt; \ > done | grep -c PASSED > < trimmed error messages > > 993 > > After: > for i in {1..10000}; do ../net/in_netns.sh ./test_tcp_rtt; \ > done | grep -c PASSED > 10000 > > Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB") > Signed-off-by: Petar Penkov <ppenkov@google.com> > --- > tools/testing/selftests/bpf/test_tcp_rtt.c | 31 ++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c > index 90c3862f74a8..2b4754473956 100644 > --- a/tools/testing/selftests/bpf/test_tcp_rtt.c > +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c > @@ -6,6 +6,7 @@ > #include <sys/types.h> > #include <sys/socket.h> > #include <netinet/in.h> > +#include <netinet/tcp.h> > #include <pthread.h> > > #include <linux/filter.h> > @@ -34,6 +35,30 @@ static void send_byte(int fd) > error(1, errno, "Failed to send single byte"); > } > > +static int wait_for_ack(int fd, int retries) > +{ > + struct tcp_info info; > + socklen_t optlen; > + int i, err; > + > + for (i = 0; i < retries; i++) { > + optlen = sizeof(info); > + err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen); > + if (err < 0) { > + log_err("Failed to lookup TCP stats"); > + return err; > + } > + > + if (info.tcpi_unacked == 0) > + return 0; > + > + sleep(1); Isn't it too big of a hammer? Maybe usleep(10) here and do x100 retries instead? > + } > + > + log_err("Did not receive ACK"); > + return -1; > +} > + > static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked, > __u32 dsack_dups, __u32 delivered, __u32 delivered_ce, > __u32 icsk_retransmits) > @@ -149,6 +174,11 @@ static int run_test(int cgroup_fd, int server_fd) > /*icsk_retransmits=*/0); > > send_byte(client_fd); > + if (wait_for_ack(client_fd, 5) < 0) { > + err = -1; > + goto close_client_fd; > + } > + > > err += verify_sk(map_fd, client_fd, "first payload byte", > /*invoked=*/2, > @@ -157,6 +187,7 @@ static int run_test(int cgroup_fd, int server_fd) > /*delivered_ce=*/0, > /*icsk_retransmits=*/0); > > +close_client_fd: > close(client_fd); > > close_bpf_object: > -- > 2.23.0.rc1.153.gdeed80330f-goog >
On Fri, Aug 16, 2019 at 9:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 08/16, Petar Penkov wrote: > > From: Petar Penkov <ppenkov@google.com> > > > > There is a race in this test between receiving the ACK for the > > single-byte packet sent in the test, and reading the values from the > > map. > > > > This patch fixes this by having the client wait until there are no more > > unacknowledged packets. > > > > Before: > > for i in {1..1000}; do ../net/in_netns.sh ./test_tcp_rtt; \ > > done | grep -c PASSED > > < trimmed error messages > > > 993 > > > > After: > > for i in {1..10000}; do ../net/in_netns.sh ./test_tcp_rtt; \ > > done | grep -c PASSED > > 10000 > > > > Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB") > > Signed-off-by: Petar Penkov <ppenkov@google.com> > > --- > > tools/testing/selftests/bpf/test_tcp_rtt.c | 31 ++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c > > index 90c3862f74a8..2b4754473956 100644 > > --- a/tools/testing/selftests/bpf/test_tcp_rtt.c > > +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c > > @@ -6,6 +6,7 @@ > > #include <sys/types.h> > > #include <sys/socket.h> > > #include <netinet/in.h> > > +#include <netinet/tcp.h> > > #include <pthread.h> > > > > #include <linux/filter.h> > > @@ -34,6 +35,30 @@ static void send_byte(int fd) > > error(1, errno, "Failed to send single byte"); > > } > > > > +static int wait_for_ack(int fd, int retries) > > +{ > > + struct tcp_info info; > > + socklen_t optlen; > > + int i, err; > > + > > + for (i = 0; i < retries; i++) { > > + optlen = sizeof(info); > > + err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen); > > + if (err < 0) { > > + log_err("Failed to lookup TCP stats"); > > + return err; > > + } > > + > > + if (info.tcpi_unacked == 0) > > + return 0; > > + > > + sleep(1); > Isn't it too big of a hammer? Maybe usleep(10) here and do x100 retries > instead? > I guess this is more consistent with the time we expect to wait for an ACK in the test, will change and send out a v2 shortly. Thank you for your quick review! > > + } > > + > > + log_err("Did not receive ACK"); > > + return -1; > > +} > > + > > static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked, > > __u32 dsack_dups, __u32 delivered, __u32 delivered_ce, > > __u32 icsk_retransmits) > > @@ -149,6 +174,11 @@ static int run_test(int cgroup_fd, int server_fd) > > /*icsk_retransmits=*/0); > > > > send_byte(client_fd); > > + if (wait_for_ack(client_fd, 5) < 0) { > > + err = -1; > > + goto close_client_fd; > > + } > > + > > > > err += verify_sk(map_fd, client_fd, "first payload byte", > > /*invoked=*/2, > > @@ -157,6 +187,7 @@ static int run_test(int cgroup_fd, int server_fd) > > /*delivered_ce=*/0, > > /*icsk_retransmits=*/0); > > > > +close_client_fd: > > close(client_fd); > > > > close_bpf_object: > > -- > > 2.23.0.rc1.153.gdeed80330f-goog > >
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c index 90c3862f74a8..2b4754473956 100644 --- a/tools/testing/selftests/bpf/test_tcp_rtt.c +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c @@ -6,6 +6,7 @@ #include <sys/types.h> #include <sys/socket.h> #include <netinet/in.h> +#include <netinet/tcp.h> #include <pthread.h> #include <linux/filter.h> @@ -34,6 +35,30 @@ static void send_byte(int fd) error(1, errno, "Failed to send single byte"); } +static int wait_for_ack(int fd, int retries) +{ + struct tcp_info info; + socklen_t optlen; + int i, err; + + for (i = 0; i < retries; i++) { + optlen = sizeof(info); + err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen); + if (err < 0) { + log_err("Failed to lookup TCP stats"); + return err; + } + + if (info.tcpi_unacked == 0) + return 0; + + sleep(1); + } + + log_err("Did not receive ACK"); + return -1; +} + static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked, __u32 dsack_dups, __u32 delivered, __u32 delivered_ce, __u32 icsk_retransmits) @@ -149,6 +174,11 @@ static int run_test(int cgroup_fd, int server_fd) /*icsk_retransmits=*/0); send_byte(client_fd); + if (wait_for_ack(client_fd, 5) < 0) { + err = -1; + goto close_client_fd; + } + err += verify_sk(map_fd, client_fd, "first payload byte", /*invoked=*/2, @@ -157,6 +187,7 @@ static int run_test(int cgroup_fd, int server_fd) /*delivered_ce=*/0, /*icsk_retransmits=*/0); +close_client_fd: close(client_fd); close_bpf_object: