Re: [PATCH v6 bpf-next 3/3] add bpf_lookup_and_delete_elem tests

[Yonghong Song <yhs@fb.com>]

On 5/5/21 2:40 AM, Denis Salopek wrote:
> Add bpf selftests and extend existing ones for a new function
> bpf_lookup_and_delete_elem() for (percpu) hash and (percpu) LRU hash map
> types.
> In test_lru_map and test_maps we add an element, lookup_and_delete it,
> then check whether it's deleted.
> The newly added lookup_and_delete prog tests practically do the same
> thing but additionally use a BPF program to change the value of the
> element for LRU maps.
> 
> Cc: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr>
> Cc: Luka Oreskovic <luka.oreskovic@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Denis Salopek <denis.salopek@sartura.hr>

Ack with a few nits below.

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

> ---
> v5: Use more appropriate macros. Better check for changed value.
> v6: Remove PERCPU macros, add ASSERT_GE macro to test_progs.h, remove
> leftover code.
> ---

Again put this right before "diff --git ..." or
in the cover letter.

>   .../bpf/prog_tests/lookup_and_delete.c        | 288 ++++++++++++++++++
>   .../bpf/progs/test_lookup_and_delete.c        |  26 ++
>   tools/testing/selftests/bpf/test_lru_map.c    |   8 +
>   tools/testing/selftests/bpf/test_maps.c       |  17 ++
>   tools/testing/selftests/bpf/test_progs.h      |  11 +
>   5 files changed, 350 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_and_delete.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c b/tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c
> new file mode 100644
> index 000000000000..b36befd37384
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <test_progs.h>
> +#include "test_lookup_and_delete.skel.h"
> +
> +#define START_VALUE 1234
> +#define NEW_VALUE 4321
> +#define MAX_ENTRIES 2
> +
> +static int duration;
> +static int nr_cpus;
> +
> +static int fill_values(int map_fd)
> +{
> +	__u64 key, value = START_VALUE;
> +	int err;
> +
> +	for (key = 1; key < MAX_ENTRIES + 1; key++) {
> +		err = bpf_map_update_elem(map_fd, &key, &value, BPF_NOEXIST);
> +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fill_values_percpu(int map_fd)
> +{
> +	__u64 key, value[nr_cpus];
> +	int i, err;
> +
> +	for (i = 0; i < nr_cpus; i++)
> +		value[i] = START_VALUE;
> +
> +	for (key = 1; key < MAX_ENTRIES + 1; key++) {
> +		err = bpf_map_update_elem(map_fd, &key, value, BPF_NOEXIST);
> +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct test_lookup_and_delete *setup_prog(enum bpf_map_type map_type,
> +						 int *map_fd)
> +{
> +	struct test_lookup_and_delete *skel;
> +	int err;
> +
> +	skel = test_lookup_and_delete__open();
> +	if (!ASSERT_OK(!skel, "test_lookup_and_delete__open"))
> +		return NULL;

Maybe just use ASSERT_OK_PTR? You used it in below function
test_lookup_and_delete_hash().

> +
> +	err = bpf_map__set_type(skel->maps.hash_map, map_type);
> +	if (!ASSERT_OK(err, "bpf_map__set_type"))
> +		goto cleanup;
> +
> +	err = bpf_map__set_max_entries(skel->maps.hash_map, MAX_ENTRIES);
> +	if (!ASSERT_OK(err, "bpf_map__set_max_entries"))
> +		goto cleanup;
> +
> +	err = test_lookup_and_delete__load(skel);
> +	if (!ASSERT_OK(err, "test_lookup_and_delete__load"))
> +		goto cleanup;
> +
> +	*map_fd = bpf_map__fd(skel->maps.hash_map);
> +	if (!ASSERT_GE(*map_fd, 0, "bpf_map__fd"))
> +		goto cleanup;
> +
> +	return skel;
> +
> +cleanup:
> +	test_lookup_and_delete__destroy(skel);
> +	return NULL;
> +}
[...]
>   	/* BPF_NOEXIST means add new element if it doesn't exist. */
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index dda52cb649dc..cae012e56d53 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -210,6 +210,17 @@ extern int test__join_cgroup(const char *path);
>   	___ok;								\
>   })
>   
> +#define ASSERT_GE(actual, expected, name) ({				\
> +	static int duration = 0;					\
> +	typeof(actual) ___act = (actual);				\
> +	typeof(expected) ___exp = (expected);				\
> +	bool ___ok = ___act >= ___exp;					\
> +	CHECK(!___ok, (name),						\
> +	      "unexpected %s: actual %lld < expected %lld\n",		\
> +	      (name), (long long)(___act), (long long)(___exp));	\
> +	___ok;								\
> +})

Andrii just added a definition ASSERT_GE in
   7a2fa70aaffc   selftests/bpf: Add remaining ASSERT_xxx() variants
   https://lore.kernel.org/bpf/20210426192949.416837-2-andrii@kernel.org

so there is no need to add ASSERT_GE any more.

> +
>   #define ASSERT_STREQ(actual, expected, name) ({				\
>   	static int duration = 0;					\
>   	const char *___act = actual;					\
>