From: Yonghong Song <yhs@fb.com>
To: Brendan Jackman <jackmanb@google.com>, <bpf@vger.kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@chromium.org>,
Florent Revest <revest@chromium.org>
Subject: Re: [PATCH 7/7] bpf: Add tests for new BPF atomic operations
Date: Mon, 23 Nov 2020 16:26:45 -0800 [thread overview]
Message-ID: <c4cf639c-f499-5179-45f4-0fe374eb7444@fb.com> (raw)
In-Reply-To: <20201123173202.1335708-8-jackmanb@google.com>
On 11/23/20 9:32 AM, Brendan Jackman wrote:
> This relies on the work done by Yonghong Song in
> https://reviews.llvm.org/D72184
>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> .../selftests/bpf/prog_tests/atomics_test.c | 145 ++++++++++++++++++
> .../selftests/bpf/progs/atomics_test.c | 61 ++++++++
> .../selftests/bpf/verifier/atomic_cmpxchg.c | 96 ++++++++++++
> .../selftests/bpf/verifier/atomic_fetch_add.c | 106 +++++++++++++
> .../selftests/bpf/verifier/atomic_xchg.c | 113 ++++++++++++++
> 6 files changed, 522 insertions(+), 1 deletion(-)
> 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_cmpxchg.c
> create mode 100644 tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
> create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xchg.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 3d5940cd110d..4e28640ca2d8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -250,7 +250,7 @@ define CLANG_BPF_BUILD_RULE
> $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
> $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \
> -c $1 -o - || echo "BPF obj compilation failed") | \
> - $(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2
> + $(LLC) -mattr=dwarfris -march=bpf -mcpu=v4 $4 -filetype=obj -o $2
We have an issue here. If we change -mcpu=v4 here, we will force
people to use trunk llvm to run selftests which is not a good idea.
I am wondering whether we can single out progs/atomics_test.c, which
will be compiled with -mcpu=v4 and run with test_progs.
test_progs-no_alu32 runs tests without alu32. Since -mcpu=v4 implies
alu32, atomic tests should be skipped in test_progs-no-alu32.
In bpf_helpers.h, we already use __clang_major__ macro to compare
to clang version,
#if __clang_major__ >= 8 && defined(__bpf__)
static __always_inline void
bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
{
if (!__builtin_constant_p(slot))
__bpf_unreachable();
...
I think we could also use __clang_major__ in progs/atomics_test.c
to enable tested intrinsics only if __clang_major__ >= 12? This
way, the same code can be compiled with -mcpu=v2/v3.
Alternatively, you can also use a macro at clang command line like
clang -mcpu=v4 -DENABLE_ATOMIC ...
clang -mcpu=v3/v2 ...
progs/atomics_test.c:
#ifdef ENABLE_ATOMIC
... atomic_intrinsics ...
#endif
> endef
> # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
> define CLANG_NOALU32_BPF_BUILD_RULE
> 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..a4859d88fc11
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +#include "atomics_test.skel.h"
> +
> +static void test_add(void)
> +{
> + struct atomics_test *atomics_skel = NULL;
> + int err, prog_fd;
> + __u32 duration = 0, retval;
> +
> + atomics_skel = atomics_test__open_and_load();
> + if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
> + goto cleanup;
> +
> + err = atomics_test__attach(atomics_skel);
> + if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
> + goto cleanup;
> +
> + 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;
> +
> + CHECK(atomics_skel->data->add64_value != 3, "add64_value",
> + "64bit atomic add value was not incremented (got %lld want 2)\n",
> + atomics_skel->data->add64_value);
> + CHECK(atomics_skel->bss->add64_result != 1, "add64_result",
> + "64bit atomic add bad return value (got %lld want 1)\n",
> + atomics_skel->bss->add64_result);
> +
> + CHECK(atomics_skel->data->add32_value != 3, "add32_value",
> + "32bit atomic add value was not incremented (got %d want 2)\n",
> + atomics_skel->data->add32_value);
> + CHECK(atomics_skel->bss->add32_result != 1, "add32_result",
> + "32bit atomic add bad return value (got %d want 1)\n",
> + atomics_skel->bss->add32_result);
> +
> + CHECK(atomics_skel->bss->add_stack_value_copy != 3, "add_stack_value",
> + "_stackbit atomic add value was not incremented (got %lld want 2)\n",
> + atomics_skel->bss->add_stack_value_copy);
> + CHECK(atomics_skel->bss->add_stack_result != 1, "add_stack_result",
> + "_stackbit atomic add bad return value (got %lld want 1)\n",
> + atomics_skel->bss->add_stack_result);
> +
> +cleanup:
> + atomics_test__destroy(atomics_skel);
> +}
> +
[...]
> diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
> new file mode 100644
> index 000000000000..d81f45eb6c45
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/atomics_test.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +__u64 add64_value = 1;
> +__u64 add64_result;
> +__u32 add32_value = 1;
> +__u32 add32_result;
> +__u64 add_stack_value_copy;
> +__u64 add_stack_result;
To please llvm10, let us initialize all unitialized globals explicitly like
__u64 add64_result = 0;
__u32 add32_result = 0;
...
llvm11 and above are okay but llvm10 put those uninitialized globals
into COM section (not .bss or .data sections) which BTF did not
handle them.
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(add, int a)
> +{
> + __u64 add_stack_value = 1;
> +
> + add64_result = __sync_fetch_and_add(&add64_value, 2);
> + add32_result = __sync_fetch_and_add(&add32_value, 2);
> + add_stack_result = __sync_fetch_and_add(&add_stack_value, 2);
> + add_stack_value_copy = add_stack_value;
> +
> + return 0;
> +}
> +
> +__u64 cmpxchg64_value = 1;
> +__u64 cmpxchg64_result_fail;
> +__u64 cmpxchg64_result_succeed;
> +__u32 cmpxchg32_value = 1;
> +__u32 cmpxchg32_result_fail;
> +__u32 cmpxchg32_result_succeed;
same here. explicitly initializing cmpxchg64_result_fail,
cmpxchg64_result_succeed, cmpxchg32_result_fail, cmpxchg32_result_succeed.
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(cmpxchg, int a)
> +{
> + cmpxchg64_result_fail = __sync_val_compare_and_swap(
> + &cmpxchg64_value, 0, 3);
> + cmpxchg64_result_succeed = __sync_val_compare_and_swap(
> + &cmpxchg64_value, 1, 2);
> +
> + cmpxchg32_result_fail = __sync_val_compare_and_swap(
> + &cmpxchg32_value, 0, 3);
> + cmpxchg32_result_succeed = __sync_val_compare_and_swap(
> + &cmpxchg32_value, 1, 2);
> +
> + return 0;
> +}
> +
> +__u64 xchg64_value = 1;
> +__u64 xchg64_result;
> +__u32 xchg32_value = 1;
> +__u32 xchg32_result;
explicitly initializing xchg64_result, xchg32_result.
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(xchg, int a)
> +{
> + __u64 val64 = 2;
> + __u32 val32 = 2;
> +
> + __atomic_exchange(&xchg64_value, &val64, &xchg64_result, __ATOMIC_RELAXED);
> + __atomic_exchange(&xchg32_value, &val32, &xchg32_result, __ATOMIC_RELAXED);
> +
> + return 0;
> +}
[...]
next prev parent reply other threads:[~2020-11-24 0:27 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-11-23 17:31 ` [PATCH 2/7] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-11-23 23:54 ` Yonghong Song
2020-11-24 11:02 ` Brendan Jackman
2020-11-24 16:04 ` Yonghong Song
2020-11-24 3:28 ` kernel test robot
2020-11-24 6:50 ` Alexei Starovoitov
2020-11-24 11:21 ` Brendan Jackman
2020-11-24 22:43 ` Alexei Starovoitov
2020-11-23 17:31 ` [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-11-23 21:12 ` kernel test robot
2020-11-23 21:51 ` kernel test robot
2020-11-24 6:52 ` Alexei Starovoitov
2020-11-24 10:48 ` Brendan Jackman
2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
2020-11-23 19:29 ` Brendan Jackman
2020-11-24 6:40 ` Alexei Starovoitov
2020-11-24 10:55 ` Brendan Jackman
2020-11-24 22:51 ` Alexei Starovoitov
2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-11-24 0:26 ` Yonghong Song [this message]
2020-11-24 13:10 ` Brendan Jackman
2020-11-23 17:36 ` [PATCH 0/7] Atomics for eBPF Brendan Jackman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c4cf639c-f499-5179-45f4-0fe374eb7444@fb.com \
--to=yhs@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jackmanb@google.com \
--cc=kpsingh@chromium.org \
--cc=revest@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).