Re: [PATCH bpf-next] bpf: selftests: remove shared header from sockmap iter test

[Yonghong Song <yhs@fb.com>]

On 9/11/20 2:14 AM, Lorenz Bauer wrote:
> The shared header to define SOCKMAP_MAX_ENTRIES is a bit overkill.
> Dynamically allocate the sock_fd array based on bpf_map__max_entries
> instead.
> 
> Suggested-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>

Ack with some nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../selftests/bpf/prog_tests/sockmap_basic.c  | 39 ++++++++++---------
>   .../selftests/bpf/progs/bpf_iter_sockmap.c    |  5 +--
>   .../selftests/bpf/progs/bpf_iter_sockmap.h    |  3 --
>   3 files changed, 23 insertions(+), 24 deletions(-)
>   delete mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 4b7a527e7e82..2672a91cd78f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -8,8 +8,6 @@
>   #include "test_sockmap_invalid_update.skel.h"
>   #include "bpf_iter_sockmap.skel.h"
>   
> -#include "progs/bpf_iter_sockmap.h"
> -
>   #define TCP_REPAIR		19	/* TCP sock is under repair right now */
>   
>   #define TCP_REPAIR_ON		1
> @@ -179,9 +177,9 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
>   	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
>   	int err, len, src_fd, iter_fd, duration = 0;
>   	union bpf_iter_link_info linfo = {0};
> -	__s64 sock_fd[SOCKMAP_MAX_ENTRIES];
> -	__u32 i, num_sockets, max_elems;
> +	__u32 i, num_sockets, num_elems;
>   	struct bpf_iter_sockmap *skel;
> +	__s64 *sock_fd = NULL;
>   	struct bpf_link *link;
>   	struct bpf_map *src;
>   	char buf[64];
> @@ -190,22 +188,26 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
>   	if (CHECK(!skel, "bpf_iter_sockmap__open_and_load", "skeleton open_and_load failed\n"))
>   		return;
>   
> -	for (i = 0; i < ARRAY_SIZE(sock_fd); i++)
> -		sock_fd[i] = -1;
> -
> -	/* Make sure we have at least one "empty" entry to test iteration of
> -	 * an empty slot.
> -	 */
> -	num_sockets = ARRAY_SIZE(sock_fd) - 1;
> -
>   	if (map_type == BPF_MAP_TYPE_SOCKMAP) {
>   		src = skel->maps.sockmap;
> -		max_elems = bpf_map__max_entries(src);
> +		num_elems = bpf_map__max_entries(src);
>   	} else {
>   		src = skel->maps.sockhash;
> -		max_elems = num_sockets;
> +		num_elems = bpf_map__max_entries(src) - 1;
>   	}
>   
> +	/* Make sure we have at least one "empty" entry to test iteration of
> +	 * an empty slot.
> +	 */
> +	num_sockets = bpf_map__max_entries(src) - 1;

I found the logic above a little bit confusing in the sense you want
num_elems and num_sockets the same for sockhash but from code they are
assigned both from bpf_map__max_entries(src) - 1. Maybe the following 
more clear?

	/* Make sure we have at least one "empty" entry to test
	 * iteration of an empty slot.
	 */
	if (map_type == BPF_MAP_TYPE_SOCKMAP) {
		src = skel->maps.sockmap;
		num_elems = bpf_map__max_entries(src);
		num_sockets = num_elems - 1;
	} else {
		src = skel->maps.sockhash;
		num_sockets = bpf_map__max_entries(src) - 1;
		num_elems = num_sockets;
	}

> +
> +	sock_fd = calloc(num_sockets, sizeof(*sock_fd));
> +	if (CHECK(!sock_fd, "calloc(sock_fd)", "failed to allocate\n"))
> +		goto out;

calloc will initialize the value to 0, but it is overwritten below
with -1. malloc is sufficient here.

> +
> +	for (i = 0; i < num_sockets; i++)
> +		sock_fd[i] = -1;
> +
>   	src_fd = bpf_map__fd(src);
>   
>   	for (i = 0; i < num_sockets; i++) {
[...]