* [bpf-next] selftests/bpf: fix race in test_tcp_rtt test
@ 2019-08-16 16:03 Petar Penkov
2019-08-16 16:13 ` Stanislav Fomichev
0 siblings, 1 reply; 3+ messages in thread
From: Petar Penkov @ 2019-08-16 16:03 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, sdf, Petar Penkov
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);
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [bpf-next] selftests/bpf: fix race in test_tcp_rtt test
2019-08-16 16:03 [bpf-next] selftests/bpf: fix race in test_tcp_rtt test Petar Penkov
@ 2019-08-16 16:13 ` Stanislav Fomichev
2019-08-16 16:37 ` Petar Penkov
0 siblings, 1 reply; 3+ messages in thread
From: Stanislav Fomichev @ 2019-08-16 16:13 UTC (permalink / raw)
To: Petar Penkov; +Cc: netdev, bpf, davem, ast, daniel, sdf, Petar Penkov
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bpf-next] selftests/bpf: fix race in test_tcp_rtt test
2019-08-16 16:13 ` Stanislav Fomichev
@ 2019-08-16 16:37 ` Petar Penkov
0 siblings, 0 replies; 3+ messages in thread
From: Petar Penkov @ 2019-08-16 16:37 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, davem, Alexei Starovoitov, Daniel Borkmann,
Stanislav Fomichev, Petar Penkov
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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-16 16:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 16:03 [bpf-next] selftests/bpf: fix race in test_tcp_rtt test Petar Penkov
2019-08-16 16:13 ` Stanislav Fomichev
2019-08-16 16:37 ` Petar Penkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.