Re: [PATCH bpf-next v3 13/14] bpf: Add tests for new BPF atomic operations

[Yonghong Song <yhs@fb.com>]

On 12/3/20 8:02 AM, Brendan Jackman wrote:
> This relies on the work done by Yonghong Song in
> https://reviews.llvm.org/D72184
> 
> Note the use of a define called ENABLE_ATOMICS_TESTS: this is used
> to:
> 
>   - Avoid breaking the build for people on old versions of Clang
>   - Avoid needing separate lists of test objects for no_alu32, where
>     atomics are not supported even if Clang has the feature.
> 
> The atomics_test.o BPF object is built unconditionally both for
> test_progs and test_progs-no_alu32. For test_progs, if Clang supports
> atomics, ENABLE_ATOMICS_TESTS is defined, so it includes the proper
> test code. Otherwise, progs and global vars are defined anyway, as
> stubs; this means that the skeleton user code still builds.
> 
> The atomics_test.o userspace object is built once and used for both
> test_progs and test_progs-no_alu32. A variable called skip_tests is
> defined in the BPF object's data section, which tells the userspace
> object whether to skip the atomics test.
> 
> Change-Id: Iecc12f35f0ded4a1dd805cce1be576e7b27917ef
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/bpf/Makefile          |   4 +
>   .../selftests/bpf/prog_tests/atomics_test.c   | 262 ++++++++++++++++++
>   .../selftests/bpf/progs/atomics_test.c        | 154 ++++++++++
>   .../selftests/bpf/verifier/atomic_and.c       |  77 +++++
>   .../selftests/bpf/verifier/atomic_cmpxchg.c   |  96 +++++++
>   .../selftests/bpf/verifier/atomic_fetch_add.c | 106 +++++++
>   .../selftests/bpf/verifier/atomic_or.c        |  77 +++++
>   .../selftests/bpf/verifier/atomic_xchg.c      |  46 +++
>   .../selftests/bpf/verifier/atomic_xor.c       |  77 +++++
>   9 files changed, 899 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/atomics_test.c
>   create mode 100644 tools/testing/selftests/bpf/progs/atomics_test.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_and.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_or.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xchg.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xor.c
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f21c4841a612..448a9eb1a56c 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -431,11 +431,15 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
>   		       $(wildcard progs/btf_dump_test_case_*.c)
>   TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>   TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
> +ifeq ($(feature-clang-bpf-atomics),1)
> +  TRUNNER_BPF_CFLAGS += -DENABLE_ATOMICS_TESTS
> +endif
>   TRUNNER_BPF_LDFLAGS := -mattr=+alu32
>   $(eval $(call DEFINE_TEST_RUNNER,test_progs))
>   
>   # Define test_progs-no_alu32 test runner.
>   TRUNNER_BPF_BUILD_RULE := CLANG_NOALU32_BPF_BUILD_RULE
> +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
>   TRUNNER_BPF_LDFLAGS :=
>   $(eval $(call DEFINE_TEST_RUNNER,test_progs,no_alu32))
>   
> diff --git a/tools/testing/selftests/bpf/prog_tests/atomics_test.c b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> new file mode 100644
> index 000000000000..66f0ccf4f4ec
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> @@ -0,0 +1,262 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +
> +#include "atomics_test.skel.h"
> +
> +static struct atomics_test *setup(void)
> +{
> +	struct atomics_test *atomics_skel;
> +	__u32 duration = 0, err;
> +
> +	atomics_skel = atomics_test__open_and_load();
> +	if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
> +		return NULL;
> +
> +	if (atomics_skel->data->skip_tests) {
> +		printf("%s:SKIP:no ENABLE_ATOMICS_TEST (missing Clang BPF atomics support)",
> +		       __func__);
> +		test__skip();
> +		goto err;
> +	}
> +
> +	err = atomics_test__attach(atomics_skel);
> +	if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
> +		goto err;
> +
> +	return atomics_skel;
> +
> +err:
> +	atomics_test__destroy(atomics_skel);
> +	return NULL;
> +}
> +
> +static void test_add(void)
> +{
> +	struct atomics_test *atomics_skel;
> +	int err, prog_fd;
> +	__u32 duration = 0, retval;
> +
> +	atomics_skel = setup();

When running the test, I observed a noticeable delay between skel load 
and skel attach. The reason is the bpf program object file contains
multiple programs and the above setup() tries to do attachment
for ALL programs but actually below only "add" program is tested.
This will unnecessarily increase test_progs running time.

The best is for setup() here only load and attach program "add".
The libbpf API bpf_program__set_autoload() can set a particular
program not autoload. You can call attach function explicitly
for one specific program. This should be able to reduce test
running time.

> +	if (!atomics_skel)
> +		return;
> +
> +	prog_fd = bpf_program__fd(atomics_skel->progs.add);
> +	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> +				NULL, NULL, &retval, &duration);
> +	if (CHECK(err || retval, "test_run add",
> +		  "err %d errno %d retval %d duration %d\n",
> +		  err, errno, retval, duration))
> +		goto cleanup;
> +
> +	ASSERT_EQ(atomics_skel->data->add64_value, 3, "add64_value");
> +	ASSERT_EQ(atomics_skel->bss->add64_result, 1, "add64_result");
> +
> +	ASSERT_EQ(atomics_skel->data->add32_value, 3, "add32_value");
> +	ASSERT_EQ(atomics_skel->bss->add32_result, 1, "add32_result");
> +
> +	ASSERT_EQ(atomics_skel->bss->add_stack_value_copy, 3, "add_stack_value");
> +	ASSERT_EQ(atomics_skel->bss->add_stack_result, 1, "add_stack_result");
> +
> +	ASSERT_EQ(atomics_skel->data->add_noreturn_value, 3, "add_noreturn_value");
> +
> +cleanup:
> +	atomics_test__destroy(atomics_skel);
> +}
> +
> +static void test_sub(void)
> +{
> +	struct atomics_test *atomics_skel;
> +	int err, prog_fd;
> +	__u32 duration = 0, retval;
> +
> +	atomics_skel = setup();
> +	if (!atomics_skel)
> +		return;
> +
[...]