bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool
@ 2020-03-13 11:31 Tobias Klauser
  2020-03-13 11:45 ` Quentin Monnet
  2020-03-13 16:52 ` Andrii Nakryiko
  0 siblings, 2 replies; 4+ messages in thread
From: Tobias Klauser @ 2020-03-13 11:31 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov
  Cc: Shuah Khan, bpf, linux-kselftest, Quentin Monnet

Commit fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for
profiler build") added a build dependency on tools/testing/selftests/bpf
to tools/bpf/bpftool. This is suboptimal with respect to a possible
stand-alone build of bpftool.

Fix this by moving
tools/testing/selftests/bpf/include/uapi/linux/types.h to
tools/include/uapi/linux/types.h

This requires an adjustment in the include search path order for the
tests in tools/testing/selftests/bpf so that tools/include/linux/types.h
is selected when building host binaries and
tools/include/uapi/linux/types.h is selected when building bpf binaries.

Verified by compiling bpftool and the bpf selftests on x86_64 with this
change.

Fixes: fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for profiler build")
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Quentin Monnet <quentin@isovalent.com>
Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 tools/bpf/bpftool/Makefile                                 | 1 -
 .../{testing/selftests/bpf => }/include/uapi/linux/types.h | 0
 tools/testing/selftests/bpf/Makefile                       | 7 ++++---
 3 files changed, 4 insertions(+), 4 deletions(-)
 rename tools/{testing/selftests/bpf => }/include/uapi/linux/types.h (100%)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 9ca3bfbb9ac4..f584d1fdfc64 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -129,7 +129,6 @@ $(OUTPUT)_bpftool: $(_OBJS) $(LIBBPF)
 skeleton/profiler.bpf.o: skeleton/profiler.bpf.c $(LIBBPF)
 	$(QUIET_CLANG)$(CLANG) \
 		-I$(srctree)/tools/include/uapi/ \
-		-I$(srctree)/tools/testing/selftests/bpf/include/uapi \
 		-I$(LIBBPF_PATH) -I$(srctree)/tools/lib \
 		-g -O2 -target bpf -c $< -o $@
 
diff --git a/tools/testing/selftests/bpf/include/uapi/linux/types.h b/tools/include/uapi/linux/types.h
similarity index 100%
rename from tools/testing/selftests/bpf/include/uapi/linux/types.h
rename to tools/include/uapi/linux/types.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index da4389dde9f7..074a05efd1ca 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -20,8 +20,9 @@ CLANG		?= clang
 LLC		?= llc
 LLVM_OBJCOPY	?= llvm-objcopy
 BPF_GCC		?= $(shell command -v bpf-gcc;)
-CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) -I$(CURDIR) -I$(APIDIR)	\
+CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) -I$(CURDIR)		\
 	  -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) -I$(TOOLSINCDIR)	\
+	  -I$(APIDIR)							\
 	  -Dbpf_prog_load=bpf_prog_test_load				\
 	  -Dbpf_load_program=bpf_test_load_program
 LDLIBS += -lcap -lelf -lz -lrt -lpthread
@@ -194,8 +195,8 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
 
 CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
 BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 			\
-	     -I$(INCLUDE_DIR) -I$(CURDIR) -I$(CURDIR)/include/uapi	\
-	     -I$(APIDIR) -I$(abspath $(OUTPUT)/../usr/include)
+	     -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR)			\
+	     -I$(abspath $(OUTPUT)/../usr/include)
 
 CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
 	       -Wno-compare-distinct-pointer-types
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool
  2020-03-13 11:31 [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool Tobias Klauser
@ 2020-03-13 11:45 ` Quentin Monnet
  2020-03-13 16:52 ` Andrii Nakryiko
  1 sibling, 0 replies; 4+ messages in thread
From: Quentin Monnet @ 2020-03-13 11:45 UTC (permalink / raw)
  To: Tobias Klauser, Daniel Borkmann, Alexei Starovoitov
  Cc: Shuah Khan, bpf, linux-kselftest

2020-03-13 12:31 UTC+0100 ~ Tobias Klauser <tklauser@distanz.ch>
> Commit fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for
> profiler build") added a build dependency on tools/testing/selftests/bpf
> to tools/bpf/bpftool. This is suboptimal with respect to a possible
> stand-alone build of bpftool.
> 
> Fix this by moving
> tools/testing/selftests/bpf/include/uapi/linux/types.h to
> tools/include/uapi/linux/types.h
> 
> This requires an adjustment in the include search path order for the
> tests in tools/testing/selftests/bpf so that tools/include/linux/types.h
> is selected when building host binaries and
> tools/include/uapi/linux/types.h is selected when building bpf binaries.
> 
> Verified by compiling bpftool and the bpf selftests on x86_64 with this
> change.

Compiles on my setup too.

Reviewed-by: Quentin Monnet <quentin@isovalent.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool
  2020-03-13 11:31 [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool Tobias Klauser
  2020-03-13 11:45 ` Quentin Monnet
@ 2020-03-13 16:52 ` Andrii Nakryiko
  2020-03-13 20:20   ` Daniel Borkmann
  1 sibling, 1 reply; 4+ messages in thread
From: Andrii Nakryiko @ 2020-03-13 16:52 UTC (permalink / raw)
  To: Tobias Klauser
  Cc: Daniel Borkmann, Alexei Starovoitov, Shuah Khan, bpf,
	open list:KERNEL SELFTEST FRAMEWORK, Quentin Monnet

On Fri, Mar 13, 2020 at 4:31 AM Tobias Klauser <tklauser@distanz.ch> wrote:
>
> Commit fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for
> profiler build") added a build dependency on tools/testing/selftests/bpf
> to tools/bpf/bpftool. This is suboptimal with respect to a possible
> stand-alone build of bpftool.
>
> Fix this by moving
> tools/testing/selftests/bpf/include/uapi/linux/types.h to
> tools/include/uapi/linux/types.h
>
> This requires an adjustment in the include search path order for the
> tests in tools/testing/selftests/bpf so that tools/include/linux/types.h
> is selected when building host binaries and
> tools/include/uapi/linux/types.h is selected when building bpf binaries.
>
> Verified by compiling bpftool and the bpf selftests on x86_64 with this
> change.
>

Thanks for following up!

My only concern is that tools/include/uapi/ is also used at least by
perf and libperf, we need to double check that they are fine with this
as well.

Given this is needed for BPF target compilation only, one way to limit
the scope of this change would be to have a `#if defined(__bpf__)`
check and falling back to "normal" uapi/linux/types.h. Alternatively,
we could have a bpf-specific subdirectory and put this header into
tools/include/bpf/uapi/linux/types.h.

I don't have any strong preferences, whatever maintainers are happy with.

> Fixes: fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for profiler build")
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Quentin Monnet <quentin@isovalent.com>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
>  tools/bpf/bpftool/Makefile                                 | 1 -
>  .../{testing/selftests/bpf => }/include/uapi/linux/types.h | 0
>  tools/testing/selftests/bpf/Makefile                       | 7 ++++---
>  3 files changed, 4 insertions(+), 4 deletions(-)
>  rename tools/{testing/selftests/bpf => }/include/uapi/linux/types.h (100%)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 9ca3bfbb9ac4..f584d1fdfc64 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -129,7 +129,6 @@ $(OUTPUT)_bpftool: $(_OBJS) $(LIBBPF)
>  skeleton/profiler.bpf.o: skeleton/profiler.bpf.c $(LIBBPF)
>         $(QUIET_CLANG)$(CLANG) \
>                 -I$(srctree)/tools/include/uapi/ \
> -               -I$(srctree)/tools/testing/selftests/bpf/include/uapi \
>                 -I$(LIBBPF_PATH) -I$(srctree)/tools/lib \
>                 -g -O2 -target bpf -c $< -o $@
>
> diff --git a/tools/testing/selftests/bpf/include/uapi/linux/types.h b/tools/include/uapi/linux/types.h
> similarity index 100%
> rename from tools/testing/selftests/bpf/include/uapi/linux/types.h
> rename to tools/include/uapi/linux/types.h
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index da4389dde9f7..074a05efd1ca 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -20,8 +20,9 @@ CLANG         ?= clang
>  LLC            ?= llc
>  LLVM_OBJCOPY   ?= llvm-objcopy
>  BPF_GCC                ?= $(shell command -v bpf-gcc;)
> -CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) -I$(CURDIR) -I$(APIDIR)   \
> +CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) -I$(CURDIR)               \
>           -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) -I$(TOOLSINCDIR)     \
> +         -I$(APIDIR)                                                   \
>           -Dbpf_prog_load=bpf_prog_test_load                            \
>           -Dbpf_load_program=bpf_test_load_program
>  LDLIBS += -lcap -lelf -lz -lrt -lpthread
> @@ -194,8 +195,8 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
>
>  CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
>  BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN)                  \
> -            -I$(INCLUDE_DIR) -I$(CURDIR) -I$(CURDIR)/include/uapi      \
> -            -I$(APIDIR) -I$(abspath $(OUTPUT)/../usr/include)
> +            -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR)                   \
> +            -I$(abspath $(OUTPUT)/../usr/include)
>
>  CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
>                -Wno-compare-distinct-pointer-types
> --
> 2.24.0
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool
  2020-03-13 16:52 ` Andrii Nakryiko
@ 2020-03-13 20:20   ` Daniel Borkmann
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2020-03-13 20:20 UTC (permalink / raw)
  To: Andrii Nakryiko, Tobias Klauser
  Cc: Alexei Starovoitov, Shuah Khan, bpf,
	open list:KERNEL SELFTEST FRAMEWORK, Quentin Monnet,
	arnaldo.melo

[ +acme ]

On 3/13/20 5:52 PM, Andrii Nakryiko wrote:
> On Fri, Mar 13, 2020 at 4:31 AM Tobias Klauser <tklauser@distanz.ch> wrote:
>>
>> Commit fe4eb069edb7 ("bpftool: Use linux/types.h from source tree for
>> profiler build") added a build dependency on tools/testing/selftests/bpf
>> to tools/bpf/bpftool. This is suboptimal with respect to a possible
>> stand-alone build of bpftool.
>>
>> Fix this by moving
>> tools/testing/selftests/bpf/include/uapi/linux/types.h to
>> tools/include/uapi/linux/types.h
>>
>> This requires an adjustment in the include search path order for the
>> tests in tools/testing/selftests/bpf so that tools/include/linux/types.h
>> is selected when building host binaries and
>> tools/include/uapi/linux/types.h is selected when building bpf binaries.
>>
>> Verified by compiling bpftool and the bpf selftests on x86_64 with this
>> change.
> 
> Thanks for following up!
> 
> My only concern is that tools/include/uapi/ is also used at least by
> perf and libperf, we need to double check that they are fine with this
> as well.
> 
> Given this is needed for BPF target compilation only, one way to limit
> the scope of this change would be to have a `#if defined(__bpf__)`
> check and falling back to "normal" uapi/linux/types.h. Alternatively,
> we could have a bpf-specific subdirectory and put this header into
> tools/include/bpf/uapi/linux/types.h.
> 
> I don't have any strong preferences, whatever maintainers are happy with.

I would prefer to keep it generic if possible before we take measures of
making special cases for bpf in tools include infra. Compilation of perf
and libperf seems fine on my side as well, so I've applied it, thanks!

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-13 20:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 11:31 [PATCH bpf-next] tools/bpf: move linux/types.h for selftests and bpftool Tobias Klauser
2020-03-13 11:45 ` Quentin Monnet
2020-03-13 16:52 ` Andrii Nakryiko
2020-03-13 20:20   ` Daniel Borkmann

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).