From: Alexander Duyck <alexander.duyck@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
John Fastabend <john.fastabend@gmail.com>,
Kernel Team <kernel-team@fb.com>, Netdev <netdev@vger.kernel.org>,
Eric Dumazet <edumazet@google.com>,
Lawrence Brakmo <brakmo@fb.com>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
alexanderduyck@fb.com
Subject: Re: [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads
Date: Mon, 2 Nov 2020 16:49:42 -0800 [thread overview]
Message-ID: <CAKgT0UceQhVGXbkZWj_aj0+Ew8oOEJMAgwAUE5GLN5EexqAhkQ@mail.gmail.com> (raw)
In-Reply-To: <20201103003836.2ngjz6yqewhn7aln@kafai-mbp.dhcp.thefacebook.com>
On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > Drop the tcp_client/server.py files in favor of using a client and server
> > thread within the test case. Specifically we spawn a new thread to play the
> The thread comment may be outdated in v2.
>
> > role of the server, and the main testing thread plays the role of client.
> >
> > Add logic to the end of the run_test function to guarantee that the sockets
> > are closed when we begin verifying results.
> >
> > Doing this we are able to reduce overhead since we don't have two python
> > workers possibly floating around. In addition we don't have to worry about
> > synchronization issues and as such the retry loop waiting for the threads
> > to close the sockets can be dropped as we will have already closed the
> > sockets in the local executable and synchronized the server thread.
> >
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > ---
> > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 96 ++++++++++++++++----
> > tools/testing/selftests/bpf/tcp_client.py | 50 ----------
> > tools/testing/selftests/bpf/tcp_server.py | 80 -----------------
> > 3 files changed, 78 insertions(+), 148 deletions(-)
> > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
> > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > index 54f1dce97729..17d4299435df 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > @@ -1,13 +1,14 @@
> > // SPDX-License-Identifier: GPL-2.0
> > #include <inttypes.h>
> > #include <test_progs.h>
> > +#include <network_helpers.h>
> >
> > #include "test_tcpbpf.h"
> >
> > +#define LO_ADDR6 "::1"
> > #define CG_NAME "/tcpbpf-user-test"
> >
> > -/* 3 comes from one listening socket + both ends of the connection */
> > -#define EXPECTED_CLOSE_EVENTS 3
> > +static __u32 duration;
> >
> > #define EXPECT_EQ(expected, actual, fmt) \
> > do { \
> > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
> > EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> > EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> > EXPECT_EQ(1, result->num_listen, PRIu32);
> > - EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
> > +
> > + /* 3 comes from one listening socket + both ends of the connection */
> > + EXPECT_EQ(3, result->num_close_events, PRIu32);
> >
> > return ret;
> > }
> > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
> > return ret;
> > }
> >
> > +static int run_test(void)
> > +{
> > + int listen_fd = -1, cli_fd = -1, accept_fd = -1;
> > + char buf[1000];
> > + int err = -1;
> > + int i;
> > +
> > + listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
> > + if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
> > + listen_fd, errno))
> > + goto done;
> > +
> > + cli_fd = connect_to_fd(listen_fd, 0);
> > + if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
> > + "cli_fd:%d errno:%d\n", cli_fd, errno))
> > + goto done;
> > +
> > + accept_fd = accept(listen_fd, NULL, NULL);
> > + if (CHECK(accept_fd == -1, "accept(listen_fd)",
> > + "accept_fd:%d errno:%d\n", accept_fd, errno))
> > + goto done;
> > +
> > + /* Send 1000B of '+'s from cli_fd -> accept_fd */
> > + for (i = 0; i < 1000; i++)
> > + buf[i] = '+';
> > +
> > + err = send(cli_fd, buf, 1000, 0);
> > + if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
> > + goto done;
> > +
> > + err = recv(accept_fd, buf, 1000, 0);
> > + if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
> > + goto done;
> > +
> > + /* Send 500B of '.'s from accept_fd ->cli_fd */
> > + for (i = 0; i < 500; i++)
> > + buf[i] = '.';
> > +
> > + err = send(accept_fd, buf, 500, 0);
> > + if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
> > + goto done;
> > +
> > + err = recv(cli_fd, buf, 500, 0);
> Unlikely, but err from the above send()/recv() could be 0.
Is that an issue? It would still trigger the check below as that is not 500.
> > + if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno))
> > + goto done;
> > +
> > + /*
> > + * shutdown accept first to guarantee correct ordering for
> > + * bytes_received and bytes_acked when we go to verify the results.
> > + */
> > + shutdown(accept_fd, SHUT_WR);
> > + err = recv(cli_fd, buf, 1, 0);
> > + if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno))
> > + goto done;
> > +
> > + shutdown(cli_fd, SHUT_WR);
> > + err = recv(accept_fd, buf, 1, 0);
> hmm... I was thinking cli_fd may still be in TCP_LAST_ACK
> but we can go with this version first and see if CI could
> really hit this case before resurrecting the idea on testing
> the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c.
I ran with this for several hours and saw no issues with over 100K
iterations all of them passing. That is why I opted to just drop the
TCP_LAST_ACK patch.
next prev parent reply other threads:[~2020-11-03 0:49 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <160416890683.710453.7723265174628409401.stgit@localhost.localdomain>
2020-10-31 18:52 ` [bpf-next PATCH v2 1/5] selftests/bpf: Move test_tcppbf_user into test_progs Alexander Duyck
2020-11-03 18:34 ` Andrii Nakryiko
2020-11-03 19:06 ` Alexander Duyck
2020-10-31 18:52 ` [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads Alexander Duyck
2020-11-03 0:38 ` Martin KaFai Lau
2020-11-03 0:49 ` Alexander Duyck [this message]
2020-11-03 1:33 ` Martin KaFai Lau
2020-11-03 16:01 ` Alexander Duyck
2020-10-31 18:52 ` [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results Alexander Duyck
2020-11-03 0:42 ` Martin KaFai Lau
2020-11-03 0:56 ` Alexander Duyck
2020-11-03 1:40 ` Martin KaFai Lau
2020-11-03 18:38 ` Andrii Nakryiko
2020-10-31 18:52 ` [bpf-next PATCH v2 4/5] selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton Alexander Duyck
2020-11-03 0:55 ` Martin KaFai Lau
2020-11-03 15:44 ` Alexander Duyck
2020-10-31 18:52 ` [bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern Alexander Duyck
2020-11-03 1:25 ` Martin KaFai Lau
2020-11-03 15:42 ` Alexander Duyck
2020-11-03 18:12 ` Martin KaFai Lau
2020-11-03 18:40 ` Andrii Nakryiko
2020-10-31 19:03 ` [bpf-next PATCH v2 0/5] selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework Alexander Duyck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKgT0UceQhVGXbkZWj_aj0+Ew8oOEJMAgwAUE5GLN5EexqAhkQ@mail.gmail.com \
--to=alexander.duyck@gmail.com \
--cc=alexanderduyck@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brakmo@fb.com \
--cc=daniel@iogearbox.net \
--cc=edumazet@google.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).