From: Brendan Jackman <jackmanb@google.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Yonghong Song <yhs@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@chromium.org>,
Florent Revest <revest@chromium.org>,
open list <linux-kernel@vger.kernel.org>,
Jann Horn <jannh@google.com>
Subject: Re: [PATCH v2 bpf-next 12/13] bpf: Add tests for new BPF atomic operations
Date: Wed, 2 Dec 2020 12:26:40 +0000 [thread overview]
Message-ID: <20201202122640.GA49766@google.com> (raw)
In-Reply-To: <CAEf4BzaAgtPazgOUQYnN9eV+TqPLtK0JTd14j5QmzeNXPZ+seQ@mail.gmail.com>
On Tue, Dec 01, 2020 at 06:22:50PM -0800, Andrii Nakryiko wrote:
> On Fri, Nov 27, 2020 at 10:01 AM Brendan Jackman <jackmanb@google.com> wrote:
[...]
> > +
> > +static void test_xchg(void)
> > +{
> > + struct atomics_test *atomics_skel = NULL;
>
> nit: = NULL is unnecessary
[...[
> > + CHECK(atomics_skel->data->xchg32_value != 2, "xchg32_value",
> > + "32bit xchg left unexpected value (got %d want 2)\n",
> > + atomics_skel->data->xchg32_value);
> > + CHECK(atomics_skel->bss->xchg32_result != 1, "xchg_result",
> > + "32bit xchg returned bad result (got %d want 1)\n",
> > + atomics_skel->bss->xchg32_result);
>
> ASSERT_EQ() is less verbose.
>
> > +
> > +cleanup:
> > + atomics_test__destroy(atomics_skel);
> > +}
> > +
> > +void test_atomics_test(void)
> > +{
>
> why the gigantic #ifdef/#else block if you could do the check here,
> skip and exit?
>
> > + test_add();
> > + test_sub();
> > + test_and();
> > + test_or();
> > + test_xor();
> > + test_cmpxchg();
> > + test_xchg();
>
>
> please model these as sub-tests, it will be easier to debug, if anything
>
> > +}
> > +
> > +#else /* ENABLE_ATOMICS_TESTS */
> > +
> > +void test_atomics_test(void)
> > +{
> > + printf("%s:SKIP:no ENABLE_ATOMICS_TEST (missing Clang BPF atomics support)",
> > + __func__);
> > + test__skip();
> > +}
> > +
> > +#endif /* ENABLE_ATOMICS_TESTS */
> > 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..3139b00937e5
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/atomics_test.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +#ifdef ENABLE_ATOMICS_TESTS
> > +
> > +__u64 add64_value = 1;
> > +__u64 add64_result = 0;
> > +__u32 add32_value = 1;
> > +__u32 add32_result = 0;
> > +__u64 add_stack_value_copy = 0;
> > +__u64 add_stack_result = 0;
>
> empty line here
>
> > +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;
> > +}
> > +
> > +__s64 sub64_value = 1;
> > +__s64 sub64_result = 0;
> > +__s32 sub32_value = 1;
> > +__s32 sub32_result = 0;
> > +__s64 sub_stack_value_copy = 0;
> > +__s64 sub_stack_result = 0;
>
> same
>
> > +SEC("fentry/bpf_fentry_test1")
> > +int BPF_PROG(sub, int a)
> > +{
> > + __u64 sub_stack_value = 1;
> > +
> > + sub64_result = __sync_fetch_and_sub(&sub64_value, 2);
> > + sub32_result = __sync_fetch_and_sub(&sub32_value, 2);
> > + sub_stack_result = __sync_fetch_and_sub(&sub_stack_value, 2);
> > + sub_stack_value_copy = sub_stack_value;
> > +
> > + return 0;
> > +}
> > +
> > +__u64 and64_value = (0x110ull << 32);
> > +__u64 and64_result = 0;
> > +__u32 and32_value = 0x110;
> > +__u32 and32_result = 0;
>
> yep
>
> > +SEC("fentry/bpf_fentry_test1")
> > +int BPF_PROG(and, int a)
> > +{
> > +
> > + and64_result = __sync_fetch_and_and(&and64_value, 0x011ull << 32);
> > + and32_result = __sync_fetch_and_and(&and32_value, 0x011);
> > +
> > + return 0;
> > +}
> > +
> > +__u64 or64_value = (0x110ull << 32);
> > +__u64 or64_result = 0;
> > +__u32 or32_value = 0x110;
> > +__u32 or32_result = 0;
>
> here too
>
> > +SEC("fentry/bpf_fentry_test1")
> > +int BPF_PROG(or, int a)
> > +{
> > + or64_result = __sync_fetch_and_or(&or64_value, 0x011ull << 32);
> > + or32_result = __sync_fetch_and_or(&or32_value, 0x011);
> > +
> > + return 0;
> > +}
> > +
> > +__u64 xor64_value = (0x110ull << 32);
> > +__u64 xor64_result = 0;
> > +__u32 xor32_value = 0x110;
> > +__u32 xor32_result = 0;
>
> you get the idea... How often do you define global variables in
> user-space code right next to the function without an extra line
> between them?..
>
[...]
> > + 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);
>
> single lines are fine here and much more readable
Thanks, ack to all comments.
next prev parent reply other threads:[~2020-12-02 12:27 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-27 17:57 [PATCH v2 bpf-next 00/13] Atomics for eBPF Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 01/13] bpf: x86: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-11-29 1:15 ` Alexei Starovoitov
2020-12-01 12:14 ` Brendan Jackman
2020-12-02 5:50 ` Alexei Starovoitov
2020-12-02 10:52 ` Brendan Jackman
2020-12-02 17:35 ` Alexei Starovoitov
2020-11-27 17:57 ` [PATCH v2 bpf-next 02/13] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-11-29 1:14 ` Alexei Starovoitov
2020-12-01 12:12 ` Brendan Jackman
2020-12-02 5:48 ` Alexei Starovoitov
2020-12-02 10:54 ` Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 03/13] bpf: x86: Factor out function to emit NEG Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 04/13] bpf: x86: Factor out a lookup table for some ALU opcodes Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 05/13] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-11-28 3:43 ` Yonghong Song
2020-12-01 12:17 ` Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 06/13] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 07/13] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-11-28 4:15 ` Yonghong Song
2020-12-01 12:22 ` Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 08/13] bpf: Add instructions for atomic_[cmp]xchg Brendan Jackman
2020-11-28 5:25 ` Yonghong Song
2020-12-01 12:27 ` Brendan Jackman
2020-11-29 1:27 ` Alexei Starovoitov
2020-12-01 12:32 ` Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 09/13] bpf: Pull out a macro for interpreting atomic ALU operations Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 10/13] bpf: Add instructions for atomic[64]_[fetch_]sub Brendan Jackman
2020-11-27 21:39 ` kernel test robot
2020-11-27 21:39 ` [RFC PATCH] bpf: bpf_atomic_alu_string[] can be static kernel test robot
2020-11-28 5:35 ` [PATCH v2 bpf-next 10/13] bpf: Add instructions for atomic[64]_[fetch_]sub Yonghong Song
2020-11-29 1:34 ` Alexei Starovoitov
2020-11-30 17:18 ` Yonghong Song
2020-12-01 12:38 ` Brendan Jackman
2020-12-02 5:55 ` Alexei Starovoitov
2020-12-02 11:19 ` Brendan Jackman
2020-11-27 17:57 ` [PATCH v2 bpf-next 11/13] bpf: Add bitwise atomic instructions Brendan Jackman
2020-11-28 5:39 ` Yonghong Song
2020-11-29 1:36 ` Alexei Starovoitov
2020-11-30 17:20 ` Yonghong Song
2020-11-27 17:57 ` [PATCH v2 bpf-next 12/13] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-12-01 3:55 ` Yonghong Song
2020-12-01 12:56 ` Brendan Jackman
2020-12-01 17:24 ` Yonghong Song
2020-12-02 2:22 ` Andrii Nakryiko
2020-12-02 12:26 ` Brendan Jackman [this message]
2020-11-27 17:57 ` [PATCH v2 bpf-next 13/13] bpf: Document new atomic instructions Brendan Jackman
2020-11-28 5:53 ` [PATCH v2 bpf-next 00/13] Atomics for eBPF Yonghong Song
2020-11-29 1:40 ` Alexei Starovoitov
2020-11-30 17:22 ` Yonghong Song
2020-12-01 3:48 ` Yonghong Song
2020-12-02 2:00 ` Andrii Nakryiko
2020-12-02 5:05 ` Yonghong Song
2020-12-02 5:53 ` John Fastabend
2020-12-02 5:59 ` Andrii Nakryiko
2020-12-02 6:27 ` John Fastabend
2020-12-02 8:03 ` Yonghong Song
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=20201202122640.GA49766@google.com \
--to=jackmanb@google.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jannh@google.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=revest@chromium.org \
--cc=yhs@fb.com \
/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).