Message-ID: <20200507060921.bns5nfxuoy5c3tcu@kafai-mbp>
Date:   Wed, 6 May 2020 23:09:21 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Stanislav Fomichev <sdf@...gle.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <ast@...nel.org>, <daniel@...earbox.net>,
        Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next v3 1/5] selftests/bpf: generalize helpers to
 control background listener

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@...com>
> Cc: Martin KaFai Lau <kafai@...com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  tools/testing/selftests/bpf/Makefile          |   2 +-
>  tools/testing/selftests/bpf/network_helpers.c | 164 ++++++++++++++++++
>  tools/testing/selftests/bpf/network_helpers.h |  12 ++
>  .../selftests/bpf/prog_tests/tcp_rtt.c        | 116 +------------
>  4 files changed, 181 insertions(+), 113 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/network_helpers.c
>  create mode 100644 tools/testing/selftests/bpf/network_helpers.h
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 3d942be23d09..8f25966b500b 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -354,7 +354,7 @@ endef
>  TRUNNER_TESTS_DIR := prog_tests
>  TRUNNER_BPF_PROGS_DIR := progs
>  TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
> -			 flow_dissector_load.h
> +			 network_helpers.c flow_dissector_load.h
>  TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
>  		       $(wildcard progs/btf_dump_test_case_*.c)
>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> new file mode 100644
> index 000000000000..6ad16dfebfb2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/err.h>
> +#include <linux/in.h>
> +#include <linux/in6.h>
> +
> +#include "network_helpers.h"
> +
> +#define CHECK_FAIL(condition) ({					\
> +	int __ret = !!(condition);					\
> +	int __save_errno = errno;					\
> +	if (__ret) {							\
> +		fprintf(stdout, "%s:FAIL:%d\n", __func__, __LINE__);	\
> +	}								\
> +	errno = __save_errno;						\
> +	__ret;								\
> +})
> +
> +#define clean_errno() (errno == 0 ? "None" : strerror(errno))
> +#define log_err(MSG, ...) fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
> +	__FILE__, __LINE__, clean_errno(), ##__VA_ARGS__)
> +
> +int start_server(int family, int type)
> +{
> +	struct sockaddr_storage addr = {};
> +	socklen_t len;
> +	int fd;
> +
> +	if (family == AF_INET) {
> +		struct sockaddr_in *sin = (void *)&addr;
> +
> +		sin->sin_family = AF_INET;
> +		len = sizeof(*sin);
> +	} else {
> +		struct sockaddr_in6 *sin6 = (void *)&addr;
> +
> +		sin6->sin6_family = AF_INET6;
> +		len = sizeof(*sin6);
> +	}
> +
> +	fd = socket(family, type | SOCK_NONBLOCK, 0);
> +	if (fd < 0) {
> +		log_err("Failed to create server socket");
> +		return -1;
> +	}
> +
> +	if (bind(fd, (const struct sockaddr *)&addr, len) < 0) {
> +		log_err("Failed to bind socket");
> +		close(fd);
> +		return -1;
> +	}
> +
> +	if (type == SOCK_STREAM) {
> +		if (listen(fd, 1) < 0) {
> +			log_err("Failed to listed on socket");
> +			close(fd);
> +			return -1;
> +		}
> +	}
> +
> +	return fd;
> +}
> +
> +static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
> +static volatile bool server_done;
> +pthread_t server_tid;
> +
> +static void *server_thread(void *arg)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +	int fd = *(int *)arg;
> +	int client_fd;
> +
> +	pthread_mutex_lock(&server_started_mtx);
> +	pthread_cond_signal(&server_started);
> +	pthread_mutex_unlock(&server_started_mtx);
> +
> +	while (true) {
> +		client_fd = accept(fd, (struct sockaddr *)&addr, &len);
> +		if (client_fd == -1 && errno == EAGAIN) {
> +			usleep(50);
> +			continue;
> +		}
> +		break;
> +	}
> +	if (CHECK_FAIL(client_fd < 0)) {
> +		perror("Failed to accept client");
> +		return ERR_PTR(-1);
> +	}
> +
> +	while (!server_done)
> +		usleep(50);
> +
> +	close(client_fd);
> +
> +	return NULL;
> +}
> +
> +int start_server_thread(int family, int type)
> +{
> +	int fd = start_server(family, type);
> +
> +	if (fd < 0)
> +		return -1;
> +
> +	if (CHECK_FAIL(pthread_create(&server_tid, NULL, server_thread,
> +				      (void *)&fd)))
> +		goto err;
> +
> +	pthread_mutex_lock(&server_started_mtx);
> +	pthread_cond_wait(&server_started, &server_started_mtx);
> +	pthread_mutex_unlock(&server_started_mtx);
> +
> +	return fd;
> +err:
> +	close(fd);
> +	return -1;
> +}
> +
> +void stop_server_thread(int fd)
> +{
> +	void *server_res;
> +
> +	server_done = true;
> +	CHECK_FAIL(pthread_join(server_tid, &server_res));
> +	CHECK_FAIL(IS_ERR(server_res));
> +	close(fd);
> +}
> +
> +int connect_to_fd(int family, int type, int server_fd)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +	int fd;
> +
> +	fd = socket(family, type, 0);
> +	if (fd < 0) {
> +		log_err("Failed to create client socket");
> +		return -1;
> +	}
> +
> +	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> +		log_err("Failed to get server addr");
> +		goto out;
> +	}
> +
> +	if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> +		log_err("Fail to connect to server with family %d", family);
> +		goto out;
> +	}
> +
> +	return fd;
> +
> +out:
> +	close(fd);
> +	return -1;
> +}
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> new file mode 100644
> index 000000000000..4ed31706b7f4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __NETWORK_HELPERS_H
> +#define __NETWORK_HELPERS_H
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +
> +int start_server(int family, int type);
> +int start_server_thread(int family, int type);
> +void stop_server_thread(int fd);
> +int connect_to_fd(int family, int type, int server_fd);
> +
> +#endif
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> index e56b52ab41da..4aaa1e6e33ad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <test_progs.h>
>  #include "cgroup_helpers.h"
> +#include "network_helpers.h"
>  
>  struct tcp_rtt_storage {
>  	__u32 invoked;
> @@ -87,34 +88,6 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
>  	return err;
>  }
>  
> -static int connect_to_server(int server_fd)
> -{
> -	struct sockaddr_storage addr;
> -	socklen_t len = sizeof(addr);
> -	int fd;
> -
> -	fd = socket(AF_INET, SOCK_STREAM, 0);
> -	if (fd < 0) {
> -		log_err("Failed to create client socket");
> -		return -1;
> -	}
> -
> -	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> -		log_err("Failed to get server addr");
> -		goto out;
> -	}
> -
> -	if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> -		log_err("Fail to connect to server");
> -		goto out;
> -	}
> -
> -	return fd;
> -
> -out:
> -	close(fd);
> -	return -1;
> -}
>  
>  static int run_test(int cgroup_fd, int server_fd)
>  {
> @@ -145,7 +118,7 @@ static int run_test(int cgroup_fd, int server_fd)
>  		goto close_bpf_object;
>  	}
>  
> -	client_fd = connect_to_server(server_fd);
> +	client_fd = connect_to_fd(AF_INET, SOCK_STREAM, server_fd);
>  	if (client_fd < 0) {
>  		err = -1;
>  		goto close_bpf_object;
> @@ -180,103 +153,22 @@ static int run_test(int cgroup_fd, int server_fd)
>  	return err;
>  }
>  
> -static int start_server(void)
> -{
> -	struct sockaddr_in addr = {
> -		.sin_family = AF_INET,
> -		.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> -	};
> -	int fd;
> -
> -	fd = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0);
> -	if (fd < 0) {
> -		log_err("Failed to create server socket");
> -		return -1;
> -	}
> -
> -	if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
> -		log_err("Failed to bind socket");
> -		close(fd);
> -		return -1;
> -	}
> -
> -	return fd;
> -}
> -
> -static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
> -static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
> -static volatile bool server_done = false;
> -
> -static void *server_thread(void *arg)
> -{
> -	struct sockaddr_storage addr;
> -	socklen_t len = sizeof(addr);
> -	int fd = *(int *)arg;
> -	int client_fd;
> -	int err;
> -
> -	err = listen(fd, 1);
> -
> -	pthread_mutex_lock(&server_started_mtx);
> -	pthread_cond_signal(&server_started);
> -	pthread_mutex_unlock(&server_started_mtx);
> -
> -	if (CHECK_FAIL(err < 0)) {
> -		perror("Failed to listed on socket");
> -		return ERR_PTR(err);
> -	}
> -
> -	while (true) {
> -		client_fd = accept(fd, (struct sockaddr *)&addr, &len);
> -		if (client_fd == -1 && errno == EAGAIN) {
> -			usleep(50);
> -			continue;
> -		}
> -		break;
> -	}
> -	if (CHECK_FAIL(client_fd < 0)) {
> -		perror("Failed to accept client");
> -		return ERR_PTR(err);
> -	}
> -
> -	while (!server_done)
> -		usleep(50);
> -
> -	close(client_fd);
> -
> -	return NULL;
> -}
> -
>  void test_tcp_rtt(void)
>  {
>  	int server_fd, cgroup_fd;
> -	pthread_t tid;
> -	void *server_res;
>  
>  	cgroup_fd = test__join_cgroup("/tcp_rtt");
>  	if (CHECK_FAIL(cgroup_fd < 0))
>  		return;
>  
> -	server_fd = start_server();
> +	server_fd = start_server_thread(AF_INET, SOCK_STREAM);
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?

Powered by blists - more mailing lists