From: Yonghong Song <yhs@fb.com>
To: Brendan Jackman <jackmanb@google.com>
Cc: <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@chromium.org>,
Florent Revest <revest@chromium.org>,
<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: Tue, 1 Dec 2020 09:24:23 -0800 [thread overview]
Message-ID: <745df1cd-d771-9eda-eb96-3731cb5da36b@fb.com> (raw)
In-Reply-To: <20201201125642.GH2114905@google.com>
On 12/1/20 4:56 AM, Brendan Jackman wrote:
> On Mon, Nov 30, 2020 at 07:55:02PM -0800, Yonghong Song wrote:
>> On 11/27/20 9:57 AM, Brendan Jackman wrote:
> [...]
>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>>> index 3d5940cd110d..5eadfd09037d 100644
>>> --- a/tools/testing/selftests/bpf/Makefile
>>> +++ b/tools/testing/selftests/bpf/Makefile
>>> @@ -228,6 +228,12 @@ IS_LITTLE_ENDIAN = $(shell $(CC) -dM -E - </dev/null | \
>>> grep 'define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__')
>>> MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
>>> +# Determine if Clang supports BPF arch v4, and therefore atomics.
>>> +CLANG_SUPPORTS_V4=$(if $(findstring v4,$(shell $(CLANG) --target=bpf -mcpu=? 2>&1)),true,)
>>> +ifeq ($(CLANG_SUPPORTS_V4),true)
>>> + CFLAGS += -DENABLE_ATOMICS_TESTS
>>> +endif
>>> +
>>> CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
>>> BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
>>> -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
>>> @@ -250,7 +256,9 @@ 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=$(if $(CLANG_SUPPORTS_V4),v4,v3) \
>>> + $4 -filetype=obj -o $2
>>> endef
>>> # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
>>> define CLANG_NOALU32_BPF_BUILD_RULE
>>> @@ -391,7 +399,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
>>> 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)
>>> +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) $(if $(CLANG_SUPPORTS_V4),-DENABLE_ATOMICS_TESTS,)
>>
>> If the compiler indeed supports cpu v4 (i.e., atomic insns),
>> -DENABLE_ATOMICS_TESTS will be added to TRUNNER_BPF_FLAGS and
>> eventually -DENABLE_ATOMICS_TESTS is also available for
>> no-alu32 test and this will cause compilation error.
>>
>> I did the following hack to workaround the issue, i.e., only adds
>> the definition to default (alu32) test run.
>>
>> index 5eadfd09037d..3d1320fd93eb 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -230,9 +230,6 @@ MENDIAN=$(if
>> $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
>>
>> # Determine if Clang supports BPF arch v4, and therefore atomics.
>> CLANG_SUPPORTS_V4=$(if $(findstring v4,$(shell $(CLANG) --target=bpf
>> -mcpu=? 2>&1)),true,)
>> -ifeq ($(CLANG_SUPPORTS_V4),true)
>> - CFLAGS += -DENABLE_ATOMICS_TESTS
>> -endif
>>
>> CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
>> BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
>> @@ -255,6 +252,7 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
>> define CLANG_BPF_BUILD_RULE
>> $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
>> $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \
>> + $(if $(CLANG_SUPPORTS_V4),-DENABLE_ATOMICS_TESTS,) \
>> -c $1 -o - || echo "BPF obj compilation failed") | \
>> $(LLC) -mattr=dwarfris -march=bpf \
>> -mcpu=$(if $(CLANG_SUPPORTS_V4),v4,v3) \
>> @@ -399,7 +397,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c
>> trace_helpers.c \
>> 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) $(if
>> $(CLANG_SUPPORTS_V4),-DENABLE_ATOMICS_TESTS,)
>> +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
>> TRUNNER_BPF_LDFLAGS := -mattr=+alu32
>> $(eval $(call DEFINE_TEST_RUNNER,test_progs))
>
> Ah, good point. I think your "hack" actually improves the overall result
> anyway since it avoids the akward global mutation of CFLAGS. Thanks!
>
> I wonder if we should actually have Clang define a built-in macro to say
> that the atomics are supported?
We are using gcc builtin's and they are all supported by clang, so
"#if __has_builtin(__sync_fetch_and_or)" is always true so it
won't work here.
We could add a macro like __BPF_ATOMICS_SUPPORTED__ in clang.
But you still need a checking to decide whether to use -mcpu=v4. If
you have that information, it will be trivial to add your
own macros if it is -mcpu=v4.
>
>>> 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..8ecc0392fdf9
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
>>> @@ -0,0 +1,329 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include <test_progs.h>
>>> +
>>> +#ifdef ENABLE_ATOMICS_TESTS
>>> +
>>> +#include "atomics_test.skel.h"
>>> +
>>> +static void test_add(void)
>> [...]
>>> +
>>> +#endif /* ENABLE_ATOMICS_TESTS */
>>> diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
> [...]
>>> +__u64 xor64_value = (0x110ull << 32);
>>> +__u64 xor64_result = 0;
>>> +__u32 xor32_value = 0x110;
>>> +__u32 xor32_result = 0;
>>> +SEC("fentry/bpf_fentry_test1")
>>> +int BPF_PROG(xor, int a)
>>> +{
>>> + xor64_result = __sync_fetch_and_xor(&xor64_value, 0x011ull << 32);
>>> + xor32_result = __sync_fetch_and_xor(&xor32_value, 0x011);
>>> +
>>> + return 0;
>>> +}
>>
>> All above __sync_fetch_and_{add, sub, and, or, xor} produces a return
>> value used later. To test atomic_<op> instructions, it will be good if
>> you can add some tests which ignores the return value.
>
> Good idea - adding an extra case to each prog. This won't assert that
> LLVM is generating "optimal" code (without BPF_FETCH) but we can at
> least get some confidence we aren't generating total garbage.
>
next prev parent reply other threads:[~2020-12-01 17:25 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 [this message]
2020-12-02 2:22 ` Andrii Nakryiko
2020-12-02 12:26 ` Brendan Jackman
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=745df1cd-d771-9eda-eb96-3731cb5da36b@fb.com \
--to=yhs@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jackmanb@google.com \
--cc=jannh@google.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.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
Powered by JetHome