| Message ID | 20201203160245.1014867-13-jackmanb@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Atomics for eBPF | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | fail | ERROR: Remove Gerrit Change-Id's before submitting upstream |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > This is somewhat cargo-culted from the libbpf build. It will be used > in a subsequent patch to query for Clang BPF atomics support. > > Change-Id: I9318a1702170eb752acced35acbb33f45126c44c Haven't seen this before. What's this Change-Id business? > Signed-off-by: Brendan Jackman <jackmanb@google.com> > --- > tools/testing/selftests/bpf/.gitignore | 1 + > tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+) All this just to detect the support for clang atomics?... Let's not pull in the entire feature-detection framework unnecessarily, selftests Makefile is complicated enough without that. [...]
On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > This is somewhat cargo-culted from the libbpf build. It will be used > > in a subsequent patch to query for Clang BPF atomics support. > > > > Change-Id: I9318a1702170eb752acced35acbb33f45126c44c > > Haven't seen this before. What's this Change-Id business? Argh, apologies. Looks like it's time for me to adopt a less error-prone workflow for sending patches. (This is noise from Gerrit, which we sometimes use for internal reviews) > > Signed-off-by: Brendan Jackman <jackmanb@google.com> > > --- > > tools/testing/selftests/bpf/.gitignore | 1 + > > tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ > > 2 files changed, 39 insertions(+) > > All this just to detect the support for clang atomics?... Let's not > pull in the entire feature-detection framework unnecessarily, > selftests Makefile is complicated enough without that. Then the test build would break for people who haven't updated Clang. Is that acceptable? I'm aware of cases where you need to be on a pretty fresh Clang for tests to _pass_ so maybe it's fine.
On Fri, Dec 4, 2020 at 1:41 AM Brendan Jackman <jackmanb@google.com> wrote: > > On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > This is somewhat cargo-culted from the libbpf build. It will be used > > > in a subsequent patch to query for Clang BPF atomics support. > > > > > > Change-Id: I9318a1702170eb752acced35acbb33f45126c44c > > > > Haven't seen this before. What's this Change-Id business? > > Argh, apologies. Looks like it's time for me to adopt a less error-prone > workflow for sending patches. > > (This is noise from Gerrit, which we sometimes use for internal reviews) > > > > Signed-off-by: Brendan Jackman <jackmanb@google.com> > > > --- > > > tools/testing/selftests/bpf/.gitignore | 1 + > > > tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ > > > 2 files changed, 39 insertions(+) > > > > All this just to detect the support for clang atomics?... Let's not > > pull in the entire feature-detection framework unnecessarily, > > selftests Makefile is complicated enough without that. > > Then the test build would break for people who haven't updated Clang. > Is that acceptable? > > I'm aware of cases where you need to be on a pretty fresh Clang for > tests to _pass_ so maybe it's fine. I didn't mean to drop any detection of this new feature. I just didn't want a new dependency on tools' feature probing framework. See IS_LITTLE_ENDIAN and get_sys_includes, we already have various feature detection-like stuff in there. So we can do this with a one-liner. I just want to keep it simple. Thanks.
On Fri, Dec 04, 2020 at 11:00:24AM -0800, Andrii Nakryiko wrote: > On Fri, Dec 4, 2020 at 1:41 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > > > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > > > This is somewhat cargo-culted from the libbpf build. It will be used > > > > in a subsequent patch to query for Clang BPF atomics support. > > > > > > > > Change-Id: I9318a1702170eb752acced35acbb33f45126c44c > > > > > > Haven't seen this before. What's this Change-Id business? > > > > Argh, apologies. Looks like it's time for me to adopt a less error-prone > > workflow for sending patches. > > > > (This is noise from Gerrit, which we sometimes use for internal reviews) > > > > > > Signed-off-by: Brendan Jackman <jackmanb@google.com> > > > > --- > > > > tools/testing/selftests/bpf/.gitignore | 1 + > > > > tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ > > > > 2 files changed, 39 insertions(+) > > > > > > All this just to detect the support for clang atomics?... Let's not > > > pull in the entire feature-detection framework unnecessarily, > > > selftests Makefile is complicated enough without that. > > > > Then the test build would break for people who haven't updated Clang. > > Is that acceptable? > > > > I'm aware of cases where you need to be on a pretty fresh Clang for > > tests to _pass_ so maybe it's fine. > > I didn't mean to drop any detection of this new feature. I just didn't > want a new dependency on tools' feature probing framework. See > IS_LITTLE_ENDIAN and get_sys_includes, we already have various feature > detection-like stuff in there. So we can do this with a one-liner. I > just want to keep it simple. Thanks. Ah right gotcha. Then yeah I think we can do this: BPF_ATOMICS_SUPPORTED = $(shell \ echo "int x = 0; int foo(void) { return __sync_val_compare_and_swap(&x, 1, 2); }" \ | $(CLANG) -x cpp-output -S -target bpf -mcpu=v3 - -o /dev/null && echo 1 || echo 0)
On Mon, Dec 7, 2020 at 3:00 AM Brendan Jackman <jackmanb@google.com> wrote: > > On Fri, Dec 04, 2020 at 11:00:24AM -0800, Andrii Nakryiko wrote: > > On Fri, Dec 4, 2020 at 1:41 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > > > > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > > > > > This is somewhat cargo-culted from the libbpf build. It will be used > > > > > in a subsequent patch to query for Clang BPF atomics support. > > > > > > > > > > Change-Id: I9318a1702170eb752acced35acbb33f45126c44c > > > > > > > > Haven't seen this before. What's this Change-Id business? > > > > > > Argh, apologies. Looks like it's time for me to adopt a less error-prone > > > workflow for sending patches. > > > > > > (This is noise from Gerrit, which we sometimes use for internal reviews) > > > > > > > > Signed-off-by: Brendan Jackman <jackmanb@google.com> > > > > > --- > > > > > tools/testing/selftests/bpf/.gitignore | 1 + > > > > > tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ > > > > > 2 files changed, 39 insertions(+) > > > > > > > > All this just to detect the support for clang atomics?... Let's not > > > > pull in the entire feature-detection framework unnecessarily, > > > > selftests Makefile is complicated enough without that. > > > > > > Then the test build would break for people who haven't updated Clang. > > > Is that acceptable? > > > > > > I'm aware of cases where you need to be on a pretty fresh Clang for > > > tests to _pass_ so maybe it's fine. > > > > I didn't mean to drop any detection of this new feature. I just didn't > > want a new dependency on tools' feature probing framework. See > > IS_LITTLE_ENDIAN and get_sys_includes, we already have various feature > > detection-like stuff in there. So we can do this with a one-liner. I > > just want to keep it simple. Thanks. > > Ah right gotcha. Then yeah I think we can do this: > > BPF_ATOMICS_SUPPORTED = $(shell \ > echo "int x = 0; int foo(void) { return __sync_val_compare_and_swap(&x, 1, 2); }" \ > | $(CLANG) -x cpp-output -S -target bpf -mcpu=v3 - -o /dev/null && echo 1 || echo 0) Looks like it would work, yes. Curious what "-x cpp-output" does?
On Mon, Dec 07, 2020 at 06:19:12PM -0800, Andrii Nakryiko wrote: > On Mon, Dec 7, 2020 at 3:00 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > On Fri, Dec 04, 2020 at 11:00:24AM -0800, Andrii Nakryiko wrote: > > > On Fri, Dec 4, 2020 at 1:41 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > > > On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > > > > > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > [...] > > > > Ah right gotcha. Then yeah I think we can do this: > > > > BPF_ATOMICS_SUPPORTED = $(shell \ > > echo "int x = 0; int foo(void) { return __sync_val_compare_and_swap(&x, 1, 2); }" \ > > | $(CLANG) -x cpp-output -S -target bpf -mcpu=v3 - -o /dev/null && echo 1 || echo 0) > > Looks like it would work, yes. / > Curious what "-x cpp-output" does? That's just to tell Clang what language to expect, since it can't infer it from a file extension: $ echo foo | clang -S - clang-10: error: -E or -x required when input is from standard input Yonghong pointed out that we can actually just use `-x c`.
On Tue, Dec 8, 2020 at 9:04 AM Brendan Jackman <jackmanb@google.com> wrote: > > On Mon, Dec 07, 2020 at 06:19:12PM -0800, Andrii Nakryiko wrote: > > On Mon, Dec 7, 2020 at 3:00 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > On Fri, Dec 04, 2020 at 11:00:24AM -0800, Andrii Nakryiko wrote: > > > > On Fri, Dec 4, 2020 at 1:41 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > > > > > On Thu, Dec 03, 2020 at 01:01:27PM -0800, Andrii Nakryiko wrote: > > > > > > On Thu, Dec 3, 2020 at 8:07 AM Brendan Jackman <jackmanb@google.com> wrote: > > > > > > > > [...] > > > > > > Ah right gotcha. Then yeah I think we can do this: > > > > > > BPF_ATOMICS_SUPPORTED = $(shell \ > > > echo "int x = 0; int foo(void) { return __sync_val_compare_and_swap(&x, 1, 2); }" \ > > > | $(CLANG) -x cpp-output -S -target bpf -mcpu=v3 - -o /dev/null && echo 1 || echo 0) > > > > Looks like it would work, yes. > / > > Curious what "-x cpp-output" does? > > That's just to tell Clang what language to expect, since it can't infer > it from a file extension: > > $ echo foo | clang -S - > clang-10: error: -E or -x required when input is from standard input > > Yonghong pointed out that we can actually just use `-x c`. yeah, that's what confused me, as we don't really write C++ for BPF code :) All good.
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 395ae040ce1f..3c604dff1e20 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -35,3 +35,4 @@ test_cpp /tools /runqslower /bench +/FEATURE-DUMP.selftests.bpf diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 894192c319fb..f21c4841a612 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -104,8 +104,46 @@ OVERRIDE_TARGETS := 1 override define CLEAN $(call msg,CLEAN) $(Q)$(RM) -r $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) $(EXTRA_CLEAN) + $(Q)$(RM) $(OUTPUT)/FEATURE-DUMP.selftests.bpf endef +# This will work when bpf is built in tools env. where srctree +# isn't set and when invoked from selftests build, where srctree +# is set to ".". building_out_of_srctree is undefined for in srctree +# builds +ifeq ($(srctree),) +update_srctree := 1 +endif +ifdef building_out_of_srctree +update_srctree := 1 +endif +ifeq ($(update_srctree),1) +srctree := $(patsubst %/,%,$(dir $(CURDIR))) +srctree := $(patsubst %/,%,$(dir $(srctree))) +srctree := $(patsubst %/,%,$(dir $(srctree))) +srctree := $(patsubst %/,%,$(dir $(srctree))) +endif + +FEATURE_USER = .selftests.bpf +FEATURE_TESTS = clang-bpf-atomics +FEATURE_DISPLAY = clang-bpf-atomics + +check_feat := 1 +NON_CHECK_FEAT_TARGETS := clean +ifdef MAKECMDGOALS +ifeq ($(filter-out $(NON_CHECK_FEAT_TARGETS),$(MAKECMDGOALS)),) + check_feat := 0 +endif +endif + +ifeq ($(check_feat),1) +ifeq ($(FEATURES_DUMP),) +include $(srctree)/tools/build/Makefile.feature +else +include $(FEATURES_DUMP) +endif +endif + include ../lib.mk SCRATCH_DIR := $(OUTPUT)/tools
This is somewhat cargo-culted from the libbpf build. It will be used in a subsequent patch to query for Clang BPF atomics support. Change-Id: I9318a1702170eb752acced35acbb33f45126c44c Signed-off-by: Brendan Jackman <jackmanb@google.com> --- tools/testing/selftests/bpf/.gitignore | 1 + tools/testing/selftests/bpf/Makefile | 38 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)