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

From: Yonghong Song
Date: Fri Dec 04 2020 - 02:07:43 EST



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@xxxxxxxxxx>
---
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;
+
[...]