[PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

-D__x86_64__ workaround was used to make /usr/include/features.h
to follow expected path through the system include headers.
This is not portable.
Instead define dummy stubs.h which is used by 'clang -target bpf'

Fixes: 6882804c916b ("selftests/bpf: add a test for overlapping packet range checks")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/Makefile    | 4 ++--
 tools/testing/selftests/bpf/gnu/stubs.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/gnu/stubs.h

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f4341e160de6..91edd0566237 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -34,6 +34,6 @@ $(BPFOBJ): force
 CLANG ?= clang
 
 %.o: %.c
-	$(CLANG) -I../../../include/uapi -I../../../../samples/bpf/ \
-		-D__x86_64__ -Wno-compare-distinct-pointer-types \
+	$(CLANG) -I. -I../../../include/uapi -I../../../../samples/bpf/ \
+		-Wno-compare-distinct-pointer-types \
 		-O2 -target bpf -c $< -o $@
diff --git a/tools/testing/selftests/bpf/gnu/stubs.h b/tools/testing/selftests/bpf/gnu/stubs.h
new file mode 100644
index 000000000000..719225b16626
--- /dev/null
+++ b/tools/testing/selftests/bpf/gnu/stubs.h
@@ -0,0 +1 @@
+/* dummy .h to trick /usr/include/features.h to work with 'clang -target bpf' */
-- 
2.9.3

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Tue, 2 May 2017 21:14:43 -0700

> -D__x86_64__ workaround was used to make /usr/include/features.h
> to follow expected path through the system include headers.
> This is not portable.
> Instead define dummy stubs.h which is used by 'clang -target bpf'
> 
> Fixes: 6882804c916b ("selftests/bpf: add a test for overlapping packet range checks")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Applied, thanks for getting rid of this wart.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 03 May 2017 09:52:51 -0400 (EDT)

> From: Alexei Starovoitov <ast@fb.com>
> Date: Tue, 2 May 2017 21:14:43 -0700
> 
>> -D__x86_64__ workaround was used to make /usr/include/features.h
>> to follow expected path through the system include headers.
>> This is not portable.
>> Instead define dummy stubs.h which is used by 'clang -target bpf'
>> 
>> Fixes: 6882804c916b ("selftests/bpf: add a test for overlapping packet range checks")
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> 
> Applied, thanks for getting rid of this wart.

Actually, we're now back to the orignal problem I had on sparc.

The issue is that you cannot resolve the core kernel __u?? types
unless __sparc__ is properly defined.

In file included from /home/davem/src/GIT/net/tools/testing/selftests/bpf/test_pkt_access.c:8:
In file included from ../../../include/uapi/linux/bpf.h:10:
/usr/include/linux/types.h:27:9: error: unknown type name '__u16'
typedef __u16 __bitwise __le16;
        ^

We really have to do the change I mentioned the first time I brought
up this issue, which is to get this via a variable set in the
per-arch top-level Makefile

Thanks.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

On 5/3/17 9:06 AM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 03 May 2017 09:52:51 -0400 (EDT)
>
>> From: Alexei Starovoitov <ast@fb.com>
>> Date: Tue, 2 May 2017 21:14:43 -0700
>>
>>> -D__x86_64__ workaround was used to make /usr/include/features.h
>>> to follow expected path through the system include headers.
>>> This is not portable.
>>> Instead define dummy stubs.h which is used by 'clang -target bpf'
>>>
>>> Fixes: 6882804c916b ("selftests/bpf: add a test for overlapping packet range checks")
>>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>>
>> Applied, thanks for getting rid of this wart.
>
> Actually, we're now back to the orignal problem I had on sparc.
>
> The issue is that you cannot resolve the core kernel __u?? types
> unless __sparc__ is properly defined.
>
> In file included from /home/davem/src/GIT/net/tools/testing/selftests/bpf/test_pkt_access.c:8:
> In file included from ../../../include/uapi/linux/bpf.h:10:
> /usr/include/linux/types.h:27:9: error: unknown type name '__u16'
> typedef __u16 __bitwise __le16;
>         ^
>
> We really have to do the change I mentioned the first time I brought
> up this issue, which is to get this via a variable set in the
> per-arch top-level Makefile

I'd really like to avoid depending on kernel configuration.
This bit:
GENDIR := ../../../../include/generated
GENHDR := $(GENDIR)/autoconf.h
is already ugly, since it doesn't work in setups that build 
out-of-source like ours.

As far as I can see
test_pkt_access.c:8 ->
tools/include/uapi/linux/bpf.h:10 ->
tools/include/linux/types.h:9 ->
/usr/include/asm/types.h -> asm-generic/int-ll64.h
as far as I can see that should be the same on most archs.
Why doesn't it work for sparc?

We can also copy paste include/uapi/asm-generic/int-ll64.h to
bpf/asm/types.h or something.
Since here we're compiling for bpf architecture adding
__x86_64__ or __sparc__ or __whatever_native_arch_here__ is incorrect.
It really should be bpf's arch types.h
Inside of it we can use '#ifdef __BPF__' to be more accurate which
is builtin macro for clang.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Wed, 3 May 2017 09:54:42 -0700

> /usr/include/asm/types.h -> asm-generic/int-ll64.h
> as far as I can see that should be the same on most archs.
> Why doesn't it work for sparc?

You can't assume anything about the kernel headers installed,
on my debian Sparc box /usr/include/asm/types.h is below.

They do things this way to facilitate multiarch building.  I think
it's pretty reasonable.

#ifndef _SPARC_TYPES_H
#define _SPARC_TYPES_H
/*
 * This file is never included by application software unless
 * explicitly requested (e.g., via linux/types.h) in which case the
 * application is Linux specific so (user-) name space pollution is
 * not a major issue.  However, for interoperability, libraries still
 * need to be careful to avoid a name clashes.
 */

#if defined(__sparc__)

#include <asm-generic/int-ll64.h>

#ifndef __ASSEMBLY__

typedef unsigned short umode_t;

#endif /* __ASSEMBLY__ */

#endif /* defined(__sparc__) */

#endif /* defined(_SPARC_TYPES_H) */

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

On 5/3/17 10:35 AM, David Miller wrote:
> From: Alexei Starovoitov <ast@fb.com>
> Date: Wed, 3 May 2017 09:54:42 -0700
>
>> /usr/include/asm/types.h -> asm-generic/int-ll64.h
>> as far as I can see that should be the same on most archs.
>> Why doesn't it work for sparc?
>
> You can't assume anything about the kernel headers installed,
> on my debian Sparc box /usr/include/asm/types.h is below.
>
> They do things this way to facilitate multiarch building.  I think
> it's pretty reasonable.
>
> #ifndef _SPARC_TYPES_H
> #define _SPARC_TYPES_H
> /*
>  * This file is never included by application software unless
>  * explicitly requested (e.g., via linux/types.h) in which case the
>  * application is Linux specific so (user-) name space pollution is
>  * not a major issue.  However, for interoperability, libraries still
>  * need to be careful to avoid a name clashes.
>  */
>
> #if defined(__sparc__)
>
> #include <asm-generic/int-ll64.h>
>
> #ifndef __ASSEMBLY__
>
> typedef unsigned short umode_t;
>
> #endif /* __ASSEMBLY__ */
>
> #endif /* defined(__sparc__) */

if it was something like
#ifdef __sparc__
...
#else
#include_next <asm/types.h>

I would buy that debian folks indeed care about multi-arch, but
what above does is making #include <linux/types.h> to be a nop
for any cross-compiler on sparc that included it.
Which is probably quite painful to debug as we found out.
You're right that we cannot assume much about /usr/include craziness.
In that sense adding __native_arch__ macro is also wrong, since
it assumes sane /usr/include without inline asm or other things
that clang for bpf arch can consume.
In that sense the only way to be independent from arch dependent
things in /usr/include is to put all arch specific headers
into our own dir in tools/selftests/ (or may be tools/bpf/include)
and point clang to that. I think the list of .h in there will be
limited. Only things like linux/types.h and gnu/stubs.h,
so it will be manageable.
Thoughts?

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Wed, 3 May 2017 20:30:22 -0700

> I would buy that debian folks indeed care about multi-arch, but
> what above does is making #include <linux/types.h> to be a nop
> for any cross-compiler on sparc that included it.

No, if you installed cross compiler for arch X it would add
another stanza doing that "ifdef __ARCH__, include blah, endif"
dance.

> You're right that we cannot assume much about /usr/include craziness.
> In that sense adding __native_arch__ macro is also wrong, since
> it assumes sane /usr/include without inline asm or other things
> that clang for bpf arch can consume.

You can assume that it's for the ARCH we are trying to run tests
for, which needs to be in the family of the kernel arch.

> In that sense the only way to be independent from arch dependent
> things in /usr/include is to put all arch specific headers
> into our own dir in tools/selftests/ (or may be tools/bpf/include)
> and point clang to that. I think the list of .h in there will be
> limited. Only things like linux/types.h and gnu/stubs.h,
> so it will be manageable.
> Thoughts?

No, this way lies madness.

If you want to get the kernel headers, set up the proper environment
instead of constantly trying to fight it.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

On 5/4/17 6:37 AM, David Miller wrote:
> From: Alexei Starovoitov <ast@fb.com>
> Date: Wed, 3 May 2017 20:30:22 -0700
>
>> I would buy that debian folks indeed care about multi-arch, but
>> what above does is making #include <linux/types.h> to be a nop
>> for any cross-compiler on sparc that included it.
>
> No, if you installed cross compiler for arch X it would add
> another stanza doing that "ifdef __ARCH__, include blah, endif"
> dance.
>
>> You're right that we cannot assume much about /usr/include craziness.
>> In that sense adding __native_arch__ macro is also wrong, since
>> it assumes sane /usr/include without inline asm or other things
>> that clang for bpf arch can consume.
>
> You can assume that it's for the ARCH we are trying to run tests
> for, which needs to be in the family of the kernel arch.
>
>> In that sense the only way to be independent from arch dependent
>> things in /usr/include is to put all arch specific headers
>> into our own dir in tools/selftests/ (or may be tools/bpf/include)
>> and point clang to that. I think the list of .h in there will be
>> limited. Only things like linux/types.h and gnu/stubs.h,
>> so it will be manageable.
>> Thoughts?
>
> No, this way lies madness.
>
> If you want to get the kernel headers, set up the proper environment
> instead of constantly trying to fight it.

We don't want to get kernel headers.
We made this mistake with samples/bpf/, since tracing actually needs
the headers to be able to call bpf_probe_read() with correct offsets.
This stuff just doesn't work on arm and not clear whether it's working
on other archs beyond x86.
For arm we had this -D__ASM_SYSREG_H hack, but it stopped working
and Andy tried to address it in [1], but it didn't go far.
So today samples/bpf/ are completely broken on arm and it's making
people believe that xdp and networking programs also need kernel
headers and also cannot work on arm, which is not the case at all.
Hence I don't want testing/selftests/bpf/ to have anything to do
with kernel headers and arch specific headers too.
All xdp programs are 90% arch independent. The only difference is
big vs little endian and clang solves this for us automatically,
since selftests's Makefile is using 'clang -target bpf' which
picks native endianness.
All headers included by tools/testing/selftests/bpf/test_*.c programs
shouldn't use anything arch specific.
They #include <linux/tcp.h> to get 'struct tcphdr' and things like
IPPROTO_IPIP and AF_INET6.
These headers should work seamlessly on all archs, but since such
headers typically do #include <linux/types.h> which is arch dependent
we get into the issue we're discussing.
We don't need native <linux/types.h>. Moreover it's incorrect to
use native types.h, since we're compiling to bpf bytecode which
is 64-bit and needs to see types.h with sizeof(void*)==8.
When we're compiling xdp programs on x86 we're compiling them
into bpf bytecode with little endian flavor. Nothing x86 specific
about it. The same bytecode will run on arm64.
That is the case for all networking programs.
Hence I think the cleanest solution is to have bpf arch's types.h
either installed with llvm/gcc or picked from selftests's dir.

Tracing side is quite different.
For example: samples/bpf/offwaketime_kern.c does:
struct task_struct *p = (void *) PT_REGS_PARM1(ctx);
u32 pid;
bpf_probe_read(&pid, sizeof(pid), &p->pid);

The '&p->pid' offset is arch and kernel specific, hence
not only we need kernel headers for the given architecture,
but autoconf.h of that specific kernel with correct kernel version too.

[1]
https://www.spinics.net/lists/arm-kernel/msg567602.html

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Thu, 4 May 2017 16:34:08 -0700

> Hence I think the cleanest solution is to have bpf arch's types.h
> either installed with llvm/gcc or picked from selftests's dir.

Something like this?

====================
[PATCH] bpf: Provide a linux/types.h override for bpf selftests.

We do not want to use the architecture's type.h header when
building BPF programs which are always 64-bit.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 tools/testing/selftests/bpf/Makefile                   | 3 ++-
 tools/testing/selftests/bpf/include/uapi/linux/types.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/include/uapi/linux/types.h

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f92f27d..f389b02 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -35,6 +35,7 @@ $(BPFOBJ): force
 CLANG ?= clang
 
 %.o: %.c
-	$(CLANG) -I. -I../../../include/uapi -I../../../../samples/bpf/ \
+	$(CLANG) -I. -I./include/uapi -I../../../include/uapi \
+		-I../../../../samples/bpf/ \
 		-Wno-compare-distinct-pointer-types \
 		-O2 -target bpf -c $< -o $@
diff --git a/tools/testing/selftests/bpf/include/uapi/linux/types.h b/tools/testing/selftests/bpf/include/uapi/linux/types.h
new file mode 100644
index 0000000..fbd16a7
--- /dev/null
+++ b/tools/testing/selftests/bpf/include/uapi/linux/types.h
@@ -0,0 +1,6 @@
+#ifndef _UAPI_LINUX_TYPES_H
+#define _UAPI_LINUX_TYPES_H
+
+#include <asm-generic/int-ll64.h>
+
+#endif /* _UAPI_LINUX_TYPES_H */
-- 
2.1.2.532.g19b5d50

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

On 5/11/17 12:02 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@fb.com>
> Date: Thu, 4 May 2017 16:34:08 -0700
>
>> Hence I think the cleanest solution is to have bpf arch's types.h
>> either installed with llvm/gcc or picked from selftests's dir.
>
> Something like this?

yes :)
Ack.

> ====================
> [PATCH] bpf: Provide a linux/types.h override for bpf selftests.
>
> We do not want to use the architecture's type.h header when
> building BPF programs which are always 64-bit.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  tools/testing/selftests/bpf/Makefile                   | 3 ++-
>  tools/testing/selftests/bpf/include/uapi/linux/types.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/include/uapi/linux/types.h
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f92f27d..f389b02 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -35,6 +35,7 @@ $(BPFOBJ): force
>  CLANG ?= clang
>
>  %.o: %.c
> -	$(CLANG) -I. -I../../../include/uapi -I../../../../samples/bpf/ \
> +	$(CLANG) -I. -I./include/uapi -I../../../include/uapi \

Can we than move gnu/stubs.h into include/uapi as well and remove
the first -I. ?
Or keep them separate, since this linux/types.h is bpf's arch types.h
whereas gnu/stubs.h is a hack for glibc /usr/include/features.h ?
I'm fine whichever way including keeping this patch as-is.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Thu, 11 May 2017 15:58:33 -0700

> Can we than move gnu/stubs.h into include/uapi as well and remove
> the first -I. ?
> Or keep them separate, since this linux/types.h is bpf's arch types.h
> whereas gnu/stubs.h is a hack for glibc /usr/include/features.h ?
> I'm fine whichever way including keeping this patch as-is.

Let's keep it like this for now, and perhaps in the long term we
can have a better more organized piece of infrastructure for this.

So I'll commit this fix for now and happily sparc now works out of the
box for all the selftests and samples as far as I can tell. :-)

This whole thing go me thinking however.  What do you expect to happen
on 32-bit architectures implementing an eBPF JIT?  That's going to
create some serious conflicts and consternation wrt.  tracing which is
going to want to use headers which are for sizeof(void *)==4 whereas
for eBPF natively it's 8.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Alexei Starovoitov]

On 5/11/17 6:29 PM, David Miller wrote:
> This whole thing go me thinking however.  What do you expect to happen
> on 32-bit architectures implementing an eBPF JIT?

I doubt any 32-bit cpu architectures will do JIT in the near future.
Doing 64-bit operations everywhere is too painful in assembler.
HW offloading is a different story. Like nfp has 32-bit registers
and everything 64-bit in bpf is not the ideal for them.
For such cases the idea is to teach verifier to recognize that
registers don't use upper 32-bits and mark them, so JITs can be
more efficient.

> That's going to
> create some serious conflicts and consternation wrt.  tracing which is
> going to want to use headers which are for sizeof(void *)==4 whereas
> for eBPF natively it's 8.

that is indeed a problem. The bpf tracing approach that includes
kernel headers mostly doesn't work on 32-bit archs, though
few iovisor/bcc scripts should be functional. Those that don't
walk pointers and relying on tracepoints instead.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[David Miller]

From: Alexei Starovoitov <ast@fb.com>
Date: Thu, 11 May 2017 22:07:04 -0700

> On 5/11/17 6:29 PM, David Miller wrote:
>> This whole thing go me thinking however.  What do you expect to happen
>> on 32-bit architectures implementing an eBPF JIT?
> 
> I doubt any 32-bit cpu architectures will do JIT in the near future.

ARM 32-bit is being implemented as we speak, in fact it's been discussed
on this very list over the past week.

Re: [PATCH net-next] selftests/bpf: get rid of -D__x86_64__

[Andrew Lunn]

On Fri, May 12, 2017 at 10:46:24AM -0400, David Miller wrote:
> From: Alexei Starovoitov <ast@fb.com>
> Date: Thu, 11 May 2017 22:07:04 -0700
> 
> > On 5/11/17 6:29 PM, David Miller wrote:
> >> This whole thing go me thinking however.  What do you expect to happen
> >> on 32-bit architectures implementing an eBPF JIT?
> > 
> > I doubt any 32-bit cpu architectures will do JIT in the near future.
> 
> ARM 32-bit is being implemented as we speak, in fact it's been discussed
> on this very list over the past week.

It could be XDP is interesting on small embedded systems, i.e ARM32
and MIPS, used in WiFi access points and the like. I've heard it said
that the Linux network stack has become too big to run well on these
systems, it no longer fits in the instruction cache. It would be
interesting to see if XDP/eBPF can be used for handling part of the
packet load without thrashing the instruction cache.

       Andrew