From: Jakub Sitnicki <jakub@cloudflare.com>
To: Martin KaFai Lau <kafai@fb.com>, Stanislav Fomichev <sdf@google.com>
Cc: <netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
<davem@davemloft.net>, <ast@kernel.org>, <daniel@iogearbox.net>,
Andrey Ignatov <rdna@fb.com>
Subject: Re: [PATCH bpf-next v3 1/5] selftests/bpf: generalize helpers to control background listener
Date: Thu, 7 May 2020 17:56:15 +0200 [thread overview]
Message-ID: <20200507175615.69909ea0@toad> (raw)
In-Reply-To: <20200507060921.bns5nfxuoy5c3tcu@kafai-mbp>
On Wed, 6 May 2020 23:09:21 -0700
Martin KaFai Lau <kafai@fb.com> wrote:
> On Wed, May 06, 2020 at 03:32:06PM -0700, Stanislav Fomichev wrote:
> > Move the following routines that let us start a background listener
> > thread and connect to a server by fd to the test_prog:
> > * start_server_thread - start background INADDR_ANY thread
> > * stop_server_thread - stop the thread
> > * connect_to_fd - connect to the server identified by fd
> >
> > These will be used in the next commit.
> >
> > Also, extend these helpers to support AF_INET6 and accept the family
> > as an argument.
> >
> > v3:
> > * export extra helper to start server without a thread (Martin KaFai Lau)
> >
> > v2:
> > * put helpers into network_helpers.c (Andrii Nakryiko)
> >
> > Cc: Andrey Ignatov <rdna@fb.com>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
Thanks for extracting network_helpers. I've seen network syscall
wrappers repeat between the tests I've touched. Will be super useful to
have it in one place.
[...]
> I still don't see a thread is needed in this existing test_tcp_rtt also.
>
> I was hoping the start/stop_server_thread() helpers can be removed.
> I am not positive the future tests will find start/stop_server_thread()
> useful as is because it only does accept() and there is no easy way to
> get the accept-ed() fd.
>
> If this test needs to keep the thread, may be only keep the
> start/stop_server_thread() in this test for now until
> there is another use case that can benefit from them.
>
> Keep the start_server() and connect_to_fd() in the new
> network_helpers.c. I think at least sk_assign.c (and likely
> a few others) should be able to reuse them later. They
> are doing something very close to start_server() and
> connect_to_fd().
>
> Thoughts?
IMHO starting a thread to accept() a connection and close it is an
overkill. I wouldn't spawn a thread if I can get away with interleaving
client and server operations on sockets on the main thread. Makes the
test code easier to follow, and strace, for me.
next prev parent reply other threads:[~2020-05-07 15:56 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-06 22:32 [PATCH bpf-next v3 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
2020-05-06 22:32 ` [PATCH bpf-next v3 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
2020-05-07 6:09 ` Martin KaFai Lau
2020-05-07 15:56 ` Jakub Sitnicki [this message]
2020-05-07 16:15 ` sdf
2020-05-06 22:32 ` [PATCH bpf-next v3 2/5] selftests/bpf: adopt accept_timeout from sockmap_listen Stanislav Fomichev
2020-05-06 22:32 ` [PATCH bpf-next v3 3/5] selftests/bpf: move existing common networking parts into network_helpers Stanislav Fomichev
2020-05-07 6:12 ` Martin KaFai Lau
2020-05-07 16:14 ` sdf
2020-05-07 16:18 ` Andrii Nakryiko
2020-05-06 22:32 ` [PATCH bpf-next v3 4/5] net: refactor arguments of inet{,6}_bind Stanislav Fomichev
2020-05-07 6:15 ` Martin KaFai Lau
2020-05-06 22:32 ` [PATCH bpf-next v3 5/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
2020-05-07 6:29 ` Martin KaFai Lau
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=20200507175615.69909ea0@toad \
--to=jakub@cloudflare.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=kafai@fb.com \
--cc=netdev@vger.kernel.org \
--cc=rdna@fb.com \
--cc=sdf@google.com \
/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).