* [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines
2019-03-20 12:53 [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Sergey Senozhatsky
@ 2019-03-20 12:53 ` Sergey Senozhatsky
2019-03-20 12:56 ` Sergey Senozhatsky
2019-03-20 12:53 ` [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons() Sergey Senozhatsky
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-20 12:53 UTC (permalink / raw)
To: Shuah Khan, Alexei Starovoitov
Cc: Stanislav Fomichev, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kselftest, netdev, bpf, Sergey Senozhatsky
Additionally, simplify bpf_ntohs/bpf_ntohl/bpf_htons/bpf_htonl
by switching to UAPI swab().
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
tools/testing/selftests/bpf/bpf_endian.h | 37 +++++-------------------
1 file changed, 8 insertions(+), 29 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index 1ed268b2002b..ba06222963d5 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,38 +20,17 @@
* use different targets.
*/
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x) __swab16(x)
-# define __bpf_htons(x) __swab16(x)
-# define __bpf_constant_ntohs(x) ___constant_swab16(x)
-# define __bpf_constant_htons(x) ___constant_swab16(x)
-# define __bpf_ntohl(x) __swab32(x)
-# define __bpf_htonl(x) __swab32(x)
-# define __bpf_constant_ntohl(x) ___constant_swab32(x)
-# define __bpf_constant_htonl(x) ___constant_swab32(x)
+# define bpf_ntohs(x) __swab16(x)
+# define bpf_htons(x) __swab16(x)
+# define bpf_ntohl(x) __swab32(x)
+# define bpf_htonl(x) __swab32(x)
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-# define __bpf_ntohs(x) (x)
-# define __bpf_htons(x) (x)
-# define __bpf_constant_ntohs(x) (x)
-# define __bpf_constant_htons(x) (x)
-# define __bpf_ntohl(x) (x)
-# define __bpf_htonl(x) (x)
-# define __bpf_constant_ntohl(x) (x)
-# define __bpf_constant_htonl(x) (x)
+# define bpf_ntohs(x) (x)
+# define bpf_htons(x) (x)
+# define bpf_ntohl(x) (x)
+# define bpf_htonl(x) (x)
#else
# error "Fix your compiler's __BYTE_ORDER__?!"
#endif
-#define bpf_htons(x) \
- (__builtin_constant_p(x) ? \
- __bpf_constant_htons(x) : __bpf_htons(x))
-#define bpf_ntohs(x) \
- (__builtin_constant_p(x) ? \
- __bpf_constant_ntohs(x) : __bpf_ntohs(x))
-#define bpf_htonl(x) \
- (__builtin_constant_p(x) ? \
- __bpf_constant_htonl(x) : __bpf_htonl(x))
-#define bpf_ntohl(x) \
- (__builtin_constant_p(x) ? \
- __bpf_constant_ntohl(x) : __bpf_ntohl(x))
-
#endif /* __BPF_ENDIAN__ */
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines
2019-03-20 12:53 ` [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines Sergey Senozhatsky
@ 2019-03-20 12:56 ` Sergey Senozhatsky
0 siblings, 0 replies; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-20 12:56 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Shuah Khan, Alexei Starovoitov, Stanislav Fomichev,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kselftest, netdev, bpf
And this one is the last one - 3/3.
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()
2019-03-20 12:53 [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Sergey Senozhatsky
2019-03-20 12:53 ` [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines Sergey Senozhatsky
@ 2019-03-20 12:53 ` Sergey Senozhatsky
2019-03-20 12:55 ` Sergey Senozhatsky
2019-03-20 17:13 ` [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Stanislav Fomichev
2019-03-21 3:24 ` Alexei Starovoitov
3 siblings, 1 reply; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-20 12:53 UTC (permalink / raw)
To: Shuah Khan, Alexei Starovoitov
Cc: Stanislav Fomichev, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kselftest, netdev, bpf, Sergey Senozhatsky
Prefer bpf_htons() instead.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
tools/testing/selftests/bpf/test_progs.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index bcbd928c96ab..a194305f98d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -28,7 +28,7 @@ static struct bpf_flow_keys pkt_v4_flow_keys = {
.thoff = sizeof(struct iphdr),
.addr_proto = ETH_P_IP,
.ip_proto = IPPROTO_TCP,
- .n_proto = __bpf_constant_htons(ETH_P_IP),
+ .n_proto = bpf_htons(ETH_P_IP),
};
static struct bpf_flow_keys pkt_v6_flow_keys = {
@@ -36,7 +36,7 @@ static struct bpf_flow_keys pkt_v6_flow_keys = {
.thoff = sizeof(struct ipv6hdr),
.addr_proto = ETH_P_IPV6,
.ip_proto = IPPROTO_TCP,
- .n_proto = __bpf_constant_htons(ETH_P_IPV6),
+ .n_proto = bpf_htons(ETH_P_IPV6),
};
void test_flow_dissector(void)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5d10aee9e277..909b339f97f4 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -11,18 +11,18 @@ int error_cnt, pass_cnt;
bool jit_enabled;
struct ipv4_packet pkt_v4 = {
- .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
+ .eth.h_proto = bpf_htons(ETH_P_IP),
.iph.ihl = 5,
.iph.protocol = IPPROTO_TCP,
- .iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
+ .iph.tot_len = bpf_htons(MAGIC_BYTES),
.tcp.urg_ptr = 123,
.tcp.doff = 5,
};
struct ipv6_packet pkt_v6 = {
- .eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
+ .eth.h_proto = bpf_htons(ETH_P_IPV6),
.iph.nexthdr = IPPROTO_TCP,
- .iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
+ .iph.payload_len = bpf_htons(MAGIC_BYTES),
.tcp.urg_ptr = 123,
.tcp.doff = 5,
};
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()
2019-03-20 12:53 ` [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons() Sergey Senozhatsky
@ 2019-03-20 12:55 ` Sergey Senozhatsky
0 siblings, 0 replies; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-20 12:55 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Shuah Khan, Alexei Starovoitov, Stanislav Fomichev,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kselftest, netdev, bpf
Yikes... This is actually - 2/3
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 12:53 [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Sergey Senozhatsky
2019-03-20 12:53 ` [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines Sergey Senozhatsky
2019-03-20 12:53 ` [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons() Sergey Senozhatsky
@ 2019-03-20 17:13 ` Stanislav Fomichev
2019-03-20 22:20 ` Yonghong Song
2019-03-21 0:22 ` Sergey Senozhatsky
2019-03-21 3:24 ` Alexei Starovoitov
3 siblings, 2 replies; 19+ messages in thread
From: Stanislav Fomichev @ 2019-03-20 17:13 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Shuah Khan, Alexei Starovoitov, Stanislav Fomichev,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kselftest, netdev, bpf
On 03/20, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
>
> (__builtin_constant_p(x) ? \
> ___constant_swab16(x) : __builtin_bswap16(x))
>
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:
>
> error: implicit declaration of function '__builtin_bswap16'
>
> We can use __builtin_bswap16() only if compiler has this built-in,
> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> __swab16()/__swab32() take care of that, and, additionally, handle
> __builtin_constant_p() cases as well:
>
> #ifdef __HAVE_BUILTIN_BSWAP16__
> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> #else
> #define __swab16(x) \
> (__builtin_constant_p((__u16)(x)) ? \
> ___constant_swab16(x) : \
> __fswab16(x))
> #endif
>
> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> __swab16()/__swab32().
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>
> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
Tested them locally with the compiler I saw the initial issues with - all
fine, I don't see any errors with the older gcc.
One last question I have is: what happens in the llvm+bpf case? Have
you tested that? I think LLVM has all the builtins required, but since
we are relying on the swab.h now (and it relies on
__HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
used from both userspace and bpf programs).
>
> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> index b25595ea4a78..1ed268b2002b 100644
> --- a/tools/testing/selftests/bpf/bpf_endian.h
> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> @@ -20,12 +20,12 @@
> * use different targets.
> */
> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -# define __bpf_ntohs(x) __builtin_bswap16(x)
> -# define __bpf_htons(x) __builtin_bswap16(x)
> +# define __bpf_ntohs(x) __swab16(x)
> +# define __bpf_htons(x) __swab16(x)
> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
> # define __bpf_constant_htons(x) ___constant_swab16(x)
> -# define __bpf_ntohl(x) __builtin_bswap32(x)
> -# define __bpf_htonl(x) __builtin_bswap32(x)
> +# define __bpf_ntohl(x) __swab32(x)
> +# define __bpf_htonl(x) __swab32(x)
> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
> # define __bpf_constant_htonl(x) ___constant_swab32(x)
> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 17:13 ` [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Stanislav Fomichev
@ 2019-03-20 22:20 ` Yonghong Song
2019-03-20 22:27 ` Stanislav Fomichev
2019-03-21 0:22 ` Sergey Senozhatsky
1 sibling, 1 reply; 19+ messages in thread
From: Yonghong Song @ 2019-03-20 22:20 UTC (permalink / raw)
To: Stanislav Fomichev, Sergey Senozhatsky
Cc: Shuah Khan, Alexei Starovoitov, Stanislav Fomichev,
Daniel Borkmann, Martin Lau, Song Liu, linux-kselftest, netdev,
bpf
On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> On 03/20, Sergey Senozhatsky wrote:
>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>> thus not all compilers are able to compile the following code:
>>
>> (__builtin_constant_p(x) ? \
>> ___constant_swab16(x) : __builtin_bswap16(x))
>>
>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>> instance:
>>
>> error: implicit declaration of function '__builtin_bswap16'
>>
>> We can use __builtin_bswap16() only if compiler has this built-in,
>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>> __swab16()/__swab32() take care of that, and, additionally, handle
>> __builtin_constant_p() cases as well:
>>
>> #ifdef __HAVE_BUILTIN_BSWAP16__
>> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>> #else
>> #define __swab16(x) \
>> (__builtin_constant_p((__u16)(x)) ? \
>> ___constant_swab16(x) : \
>> __fswab16(x))
>> #endif
>>
>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>> __swab16()/__swab32().
>>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> ---
>>
>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.
>
> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).
Inside kernel clang compiler header (linux/compiler-clang.h) does not
define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
the above. So I think it should work with clang + bpf.
>
>>
>> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>> index b25595ea4a78..1ed268b2002b 100644
>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>> @@ -20,12 +20,12 @@
>> * use different targets.
>> */
>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> -# define __bpf_ntohs(x) __builtin_bswap16(x)
>> -# define __bpf_htons(x) __builtin_bswap16(x)
>> +# define __bpf_ntohs(x) __swab16(x)
>> +# define __bpf_htons(x) __swab16(x)
>> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
>> # define __bpf_constant_htons(x) ___constant_swab16(x)
>> -# define __bpf_ntohl(x) __builtin_bswap32(x)
>> -# define __bpf_htonl(x) __builtin_bswap32(x)
>> +# define __bpf_ntohl(x) __swab32(x)
>> +# define __bpf_htonl(x) __swab32(x)
>> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
>> # define __bpf_constant_htonl(x) ___constant_swab32(x)
>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> --
>> 2.21.0
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 22:20 ` Yonghong Song
@ 2019-03-20 22:27 ` Stanislav Fomichev
2019-03-20 22:45 ` Yonghong Song
2019-03-21 0:34 ` Sergey Senozhatsky
0 siblings, 2 replies; 19+ messages in thread
From: Stanislav Fomichev @ 2019-03-20 22:27 UTC (permalink / raw)
To: Yonghong Song
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Daniel Borkmann, Martin Lau, Song Liu,
linux-kselftest, netdev, bpf
On 03/20, Yonghong Song wrote:
>
>
> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> > On 03/20, Sergey Senozhatsky wrote:
> >> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >> thus not all compilers are able to compile the following code:
> >>
> >> (__builtin_constant_p(x) ? \
> >> ___constant_swab16(x) : __builtin_bswap16(x))
> >>
> >> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >> instance:
> >>
> >> error: implicit declaration of function '__builtin_bswap16'
> >>
> >> We can use __builtin_bswap16() only if compiler has this built-in,
> >> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >> __swab16()/__swab32() take care of that, and, additionally, handle
> >> __builtin_constant_p() cases as well:
> >>
> >> #ifdef __HAVE_BUILTIN_BSWAP16__
> >> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >> #else
> >> #define __swab16(x) \
> >> (__builtin_constant_p((__u16)(x)) ? \
> >> ___constant_swab16(x) : \
> >> __fswab16(x))
> >> #endif
> >>
> >> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >> __swab16()/__swab32().
> >>
> >> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >> ---
> >>
> >> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> > Tested them locally with the compiler I saw the initial issues with - all
> > fine, I don't see any errors with the older gcc.
> >
> > One last question I have is: what happens in the llvm+bpf case? Have
> > you tested that? I think LLVM has all the builtins required, but since
> > we are relying on the swab.h now (and it relies on
> > __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> > correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> > used from both userspace and bpf programs).
>
> Inside kernel clang compiler header (linux/compiler-clang.h) does not
> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> the above. So I think it should work with clang + bpf.
Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
want to use the builtins to make it properly generate
BPF_TO_BE/BPF_TO_LE instructions.
> >
> >>
> >> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >> index b25595ea4a78..1ed268b2002b 100644
> >> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >> @@ -20,12 +20,12 @@
> >> * use different targets.
> >> */
> >> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >> -# define __bpf_ntohs(x) __builtin_bswap16(x)
> >> -# define __bpf_htons(x) __builtin_bswap16(x)
> >> +# define __bpf_ntohs(x) __swab16(x)
> >> +# define __bpf_htons(x) __swab16(x)
> >> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
> >> # define __bpf_constant_htons(x) ___constant_swab16(x)
> >> -# define __bpf_ntohl(x) __builtin_bswap32(x)
> >> -# define __bpf_htonl(x) __builtin_bswap32(x)
> >> +# define __bpf_ntohl(x) __swab32(x)
> >> +# define __bpf_htonl(x) __swab32(x)
> >> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
> >> # define __bpf_constant_htonl(x) ___constant_swab32(x)
> >> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >> --
> >> 2.21.0
> >>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 22:27 ` Stanislav Fomichev
@ 2019-03-20 22:45 ` Yonghong Song
2019-03-20 23:08 ` Daniel Borkmann
2019-03-21 0:34 ` Sergey Senozhatsky
1 sibling, 1 reply; 19+ messages in thread
From: Yonghong Song @ 2019-03-20 22:45 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Daniel Borkmann, Martin Lau, Song Liu,
linux-kselftest, netdev, bpf
On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> On 03/20, Yonghong Song wrote:
>>
>>
>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>> On 03/20, Sergey Senozhatsky wrote:
>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>> thus not all compilers are able to compile the following code:
>>>>
>>>> (__builtin_constant_p(x) ? \
>>>> ___constant_swab16(x) : __builtin_bswap16(x))
>>>>
>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>> instance:
>>>>
>>>> error: implicit declaration of function '__builtin_bswap16'
>>>>
>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>> __builtin_constant_p() cases as well:
>>>>
>>>> #ifdef __HAVE_BUILTIN_BSWAP16__
>>>> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>> #else
>>>> #define __swab16(x) \
>>>> (__builtin_constant_p((__u16)(x)) ? \
>>>> ___constant_swab16(x) : \
>>>> __fswab16(x))
>>>> #endif
>>>>
>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>> __swab16()/__swab32().
>>>>
>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>> ---
>>>>
>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>> Tested them locally with the compiler I saw the initial issues with - all
>>> fine, I don't see any errors with the older gcc.
>>>
>>> One last question I have is: what happens in the llvm+bpf case? Have
>>> you tested that? I think LLVM has all the builtins required, but since
>>> we are relying on the swab.h now (and it relies on
>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>> used from both userspace and bpf programs).
>>
>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>> the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.
Okay, I see. Then this patch will not achieve that.
The following are two common ways to compile a bpf program:
- "clang -target bpf ...", maybe add macro __BPF__ somewhere
to indicate builtin_bswap16 always available?
- "clang <host target> ..." and then "llc -march=bpf ..."
in this case, __BPF__ macro is not available and
we will not be able to use builtin swap for bpf program.
Maybe use __clang__ macro (or gcc macro) to distinguish between clang
and gcc. If it is gcc we will check builtin availability, otherwise,
we assume builtin always available? This not pretty though.
>
>>>
>>>>
>>>> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>> index b25595ea4a78..1ed268b2002b 100644
>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>> @@ -20,12 +20,12 @@
>>>> * use different targets.
>>>> */
>>>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> -# define __bpf_ntohs(x) __builtin_bswap16(x)
>>>> -# define __bpf_htons(x) __builtin_bswap16(x)
>>>> +# define __bpf_ntohs(x) __swab16(x)
>>>> +# define __bpf_htons(x) __swab16(x)
>>>> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
>>>> # define __bpf_constant_htons(x) ___constant_swab16(x)
>>>> -# define __bpf_ntohl(x) __builtin_bswap32(x)
>>>> -# define __bpf_htonl(x) __builtin_bswap32(x)
>>>> +# define __bpf_ntohl(x) __swab32(x)
>>>> +# define __bpf_htonl(x) __swab32(x)
>>>> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
>>>> # define __bpf_constant_htonl(x) ___constant_swab32(x)
>>>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>> --
>>>> 2.21.0
>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 22:45 ` Yonghong Song
@ 2019-03-20 23:08 ` Daniel Borkmann
2019-03-21 0:03 ` Stanislav Fomichev
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Borkmann @ 2019-03-20 23:08 UTC (permalink / raw)
To: Yonghong Song, Stanislav Fomichev
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Martin Lau, Song Liu, linux-kselftest,
netdev, bpf
On 03/20/2019 11:45 PM, Yonghong Song wrote:
> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>> On 03/20, Yonghong Song wrote:
>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>> thus not all compilers are able to compile the following code:
>>>>>
>>>>> (__builtin_constant_p(x) ? \
>>>>> ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>
>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>> instance:
>>>>>
>>>>> error: implicit declaration of function '__builtin_bswap16'
>>>>>
>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>> __builtin_constant_p() cases as well:
>>>>>
>>>>> #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>> #else
>>>>> #define __swab16(x) \
>>>>> (__builtin_constant_p((__u16)(x)) ? \
>>>>> ___constant_swab16(x) : \
>>>>> __fswab16(x))
>>>>> #endif
>>>>>
>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>> __swab16()/__swab32().
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>> ---
>>>>>
>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>> fine, I don't see any errors with the older gcc.
>>>>
>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>> you tested that? I think LLVM has all the builtins required, but since
>>>> we are relying on the swab.h now (and it relies on
>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>> used from both userspace and bpf programs).
>>>
>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>> the above. So I think it should work with clang + bpf.
>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>> want to use the builtins to make it properly generate
>> BPF_TO_BE/BPF_TO_LE instructions.
>
> Okay, I see. Then this patch will not achieve that.
> The following are two common ways to compile a bpf program:
> - "clang -target bpf ...", maybe add macro __BPF__ somewhere
> to indicate builtin_bswap16 always available?
> - "clang <host target> ..." and then "llc -march=bpf ..."
> in this case, __BPF__ macro is not available and
> we will not be able to use builtin swap for bpf program.
>
> Maybe use __clang__ macro (or gcc macro) to distinguish between clang
> and gcc. If it is gcc we will check builtin availability, otherwise,
> we assume builtin always available? This not pretty though.
I think the way this should be fixed is the following: In case
of LLVM (aka compiling BPF prog), we want the code to be as-is,
in case if gcc is compiling the hostprog, we either want to keep
using __builtin_bswap16() or fall-back to something else. Thus,
I would suggest, we add a new feature test for tooling infra under
tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
which either resolves to __builtin_bswap16() or some fallback
implementation if not available. I don't think there should be much
of an issue and it would follow the standard way to do it.
>>>>> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> @@ -20,12 +20,12 @@
>>>>> * use different targets.
>>>>> */
>>>>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> -# define __bpf_ntohs(x) __builtin_bswap16(x)
>>>>> -# define __bpf_htons(x) __builtin_bswap16(x)
>>>>> +# define __bpf_ntohs(x) __swab16(x)
>>>>> +# define __bpf_htons(x) __swab16(x)
>>>>> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
>>>>> # define __bpf_constant_htons(x) ___constant_swab16(x)
>>>>> -# define __bpf_ntohl(x) __builtin_bswap32(x)
>>>>> -# define __bpf_htonl(x) __builtin_bswap32(x)
>>>>> +# define __bpf_ntohl(x) __swab32(x)
>>>>> +# define __bpf_htonl(x) __swab32(x)
>>>>> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
>>>>> # define __bpf_constant_htonl(x) ___constant_swab32(x)
>>>>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>> --
>>>>> 2.21.0
>>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 23:08 ` Daniel Borkmann
@ 2019-03-21 0:03 ` Stanislav Fomichev
2019-03-21 0:23 ` Yonghong Song
0 siblings, 1 reply; 19+ messages in thread
From: Stanislav Fomichev @ 2019-03-21 0:03 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Yonghong Song, Sergey Senozhatsky, Shuah Khan,
Alexei Starovoitov, Stanislav Fomichev, Martin Lau, Song Liu,
linux-kselftest, netdev, bpf
On 03/21, Daniel Borkmann wrote:
> On 03/20/2019 11:45 PM, Yonghong Song wrote:
> > On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> >> On 03/20, Yonghong Song wrote:
> >>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> >>>> On 03/20, Sergey Senozhatsky wrote:
> >>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >>>>> thus not all compilers are able to compile the following code:
> >>>>>
> >>>>> (__builtin_constant_p(x) ? \
> >>>>> ___constant_swab16(x) : __builtin_bswap16(x))
> >>>>>
> >>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >>>>> instance:
> >>>>>
> >>>>> error: implicit declaration of function '__builtin_bswap16'
> >>>>>
> >>>>> We can use __builtin_bswap16() only if compiler has this built-in,
> >>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >>>>> __swab16()/__swab32() take care of that, and, additionally, handle
> >>>>> __builtin_constant_p() cases as well:
> >>>>>
> >>>>> #ifdef __HAVE_BUILTIN_BSWAP16__
> >>>>> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>>>> #else
> >>>>> #define __swab16(x) \
> >>>>> (__builtin_constant_p((__u16)(x)) ? \
> >>>>> ___constant_swab16(x) : \
> >>>>> __fswab16(x))
> >>>>> #endif
> >>>>>
> >>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >>>>> __swab16()/__swab32().
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >>>>> ---
> >>>>>
> >>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> >>>> Tested them locally with the compiler I saw the initial issues with - all
> >>>> fine, I don't see any errors with the older gcc.
> >>>>
> >>>> One last question I have is: what happens in the llvm+bpf case? Have
> >>>> you tested that? I think LLVM has all the builtins required, but since
> >>>> we are relying on the swab.h now (and it relies on
> >>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> >>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> >>>> used from both userspace and bpf programs).
> >>>
> >>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
> >>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> >>> the above. So I think it should work with clang + bpf.
> >> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> >> want to use the builtins to make it properly generate
> >> BPF_TO_BE/BPF_TO_LE instructions.
> >
> > Okay, I see. Then this patch will not achieve that.
> > The following are two common ways to compile a bpf program:
> > - "clang -target bpf ...", maybe add macro __BPF__ somewhere
> > to indicate builtin_bswap16 always available?
> > - "clang <host target> ..." and then "llc -march=bpf ..."
> > in this case, __BPF__ macro is not available and
> > we will not be able to use builtin swap for bpf program.
> >
> > Maybe use __clang__ macro (or gcc macro) to distinguish between clang
> > and gcc. If it is gcc we will check builtin availability, otherwise,
> > we assume builtin always available? This not pretty though.
>
> I think the way this should be fixed is the following: In case
> of LLVM (aka compiling BPF prog), we want the code to be as-is,
> in case if gcc is compiling the hostprog, we either want to keep
> using __builtin_bswap16() or fall-back to something else. Thus,
> I would suggest, we add a new feature test for tooling infra under
> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
> which either resolves to __builtin_bswap16() or some fallback
> implementation if not available. I don't think there should be much
> of an issue and it would follow the standard way to do it.
It's not as easy as llvm vs gcc. We can compile userland tests with
llvm/clang as well. We really need to distinguish between the target: bfp vs
non-bpf: always use builtins in bpf case and fallback to swab.h for
userland (or use feature detection, but swab.h should be enough in
theory).
Can we rely on __bpf__ define?
$ cat tmp.c
#ifdef __bpf__
#error a
#else
#error b
#endif
$ clang -c -target bpf tmp.c
tmp.c:2:2: error: a
#error a
^
1 error generated.
>
> >>>>> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> index b25595ea4a78..1ed268b2002b 100644
> >>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> @@ -20,12 +20,12 @@
> >>>>> * use different targets.
> >>>>> */
> >>>>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >>>>> -# define __bpf_ntohs(x) __builtin_bswap16(x)
> >>>>> -# define __bpf_htons(x) __builtin_bswap16(x)
> >>>>> +# define __bpf_ntohs(x) __swab16(x)
> >>>>> +# define __bpf_htons(x) __swab16(x)
> >>>>> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
> >>>>> # define __bpf_constant_htons(x) ___constant_swab16(x)
> >>>>> -# define __bpf_ntohl(x) __builtin_bswap32(x)
> >>>>> -# define __bpf_htonl(x) __builtin_bswap32(x)
> >>>>> +# define __bpf_ntohl(x) __swab32(x)
> >>>>> +# define __bpf_htonl(x) __swab32(x)
> >>>>> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
> >>>>> # define __bpf_constant_htonl(x) ___constant_swab32(x)
> >>>>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >>>>> --
> >>>>> 2.21.0
> >>>>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-21 0:03 ` Stanislav Fomichev
@ 2019-03-21 0:23 ` Yonghong Song
0 siblings, 0 replies; 19+ messages in thread
From: Yonghong Song @ 2019-03-21 0:23 UTC (permalink / raw)
To: Stanislav Fomichev, Daniel Borkmann
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Martin Lau, Song Liu, linux-kselftest,
netdev, bpf
On 3/20/19 5:03 PM, Stanislav Fomichev wrote:
> On 03/21, Daniel Borkmann wrote:
>> On 03/20/2019 11:45 PM, Yonghong Song wrote:
>>> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>>>> On 03/20, Yonghong Song wrote:
>>>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>>>> thus not all compilers are able to compile the following code:
>>>>>>>
>>>>>>> (__builtin_constant_p(x) ? \
>>>>>>> ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>>>
>>>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>>>> instance:
>>>>>>>
>>>>>>> error: implicit declaration of function '__builtin_bswap16'
>>>>>>>
>>>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>>>> __builtin_constant_p() cases as well:
>>>>>>>
>>>>>>> #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>>> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>>> #else
>>>>>>> #define __swab16(x) \
>>>>>>> (__builtin_constant_p((__u16)(x)) ? \
>>>>>>> ___constant_swab16(x) : \
>>>>>>> __fswab16(x))
>>>>>>> #endif
>>>>>>>
>>>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>>>> __swab16()/__swab32().
>>>>>>>
>>>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>>>> fine, I don't see any errors with the older gcc.
>>>>>>
>>>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>>>> you tested that? I think LLVM has all the builtins required, but since
>>>>>> we are relying on the swab.h now (and it relies on
>>>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>>>> used from both userspace and bpf programs).
>>>>>
>>>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>>>> the above. So I think it should work with clang + bpf.
>>>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>>>> want to use the builtins to make it properly generate
>>>> BPF_TO_BE/BPF_TO_LE instructions.
>>>
>>> Okay, I see. Then this patch will not achieve that.
>>> The following are two common ways to compile a bpf program:
>>> - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>>> to indicate builtin_bswap16 always available?
>>> - "clang <host target> ..." and then "llc -march=bpf ..."
>>> in this case, __BPF__ macro is not available and
>>> we will not be able to use builtin swap for bpf program.
>>>
>>> Maybe use __clang__ macro (or gcc macro) to distinguish between clang
>>> and gcc. If it is gcc we will check builtin availability, otherwise,
>>> we assume builtin always available? This not pretty though.
>>
>> I think the way this should be fixed is the following: In case
>> of LLVM (aka compiling BPF prog), we want the code to be as-is,
>> in case if gcc is compiling the hostprog, we either want to keep
>> using __builtin_bswap16() or fall-back to something else. Thus,
>> I would suggest, we add a new feature test for tooling infra under
>> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
>> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
>> which either resolves to __builtin_bswap16() or some fallback
>> implementation if not available. I don't think there should be much
>> of an issue and it would follow the standard way to do it.
> It's not as easy as llvm vs gcc. We can compile userland tests with
> llvm/clang as well. We really need to distinguish between the target: bfp vs
> non-bpf: always use builtins in bpf case and fallback to swab.h for
> userland (or use feature detection, but swab.h should be enough in
> theory).
>
> Can we rely on __bpf__ define?
>
> $ cat tmp.c
> #ifdef __bpf__
> #error a
> #else
> #error b
> #endif
> $ clang -c -target bpf tmp.c
> tmp.c:2:2: error: a
> #error a
> ^
> 1 error generated.
Yes, you can rely this, __bpf__, __bpf or __BPF__. These three
are clang predefined macros for target bpf.
>
>>
>>>>>>> tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> @@ -20,12 +20,12 @@
>>>>>>> * use different targets.
>>>>>>> */
>>>>>>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>>> -# define __bpf_ntohs(x) __builtin_bswap16(x)
>>>>>>> -# define __bpf_htons(x) __builtin_bswap16(x)
>>>>>>> +# define __bpf_ntohs(x) __swab16(x)
>>>>>>> +# define __bpf_htons(x) __swab16(x)
>>>>>>> # define __bpf_constant_ntohs(x) ___constant_swab16(x)
>>>>>>> # define __bpf_constant_htons(x) ___constant_swab16(x)
>>>>>>> -# define __bpf_ntohl(x) __builtin_bswap32(x)
>>>>>>> -# define __bpf_htonl(x) __builtin_bswap32(x)
>>>>>>> +# define __bpf_ntohl(x) __swab32(x)
>>>>>>> +# define __bpf_htonl(x) __swab32(x)
>>>>>>> # define __bpf_constant_ntohl(x) ___constant_swab32(x)
>>>>>>> # define __bpf_constant_htonl(x) ___constant_swab32(x)
>>>>>>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>>>> --
>>>>>>> 2.21.0
>>>>>>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 22:27 ` Stanislav Fomichev
2019-03-20 22:45 ` Yonghong Song
@ 2019-03-21 0:34 ` Sergey Senozhatsky
1 sibling, 0 replies; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-21 0:34 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Yonghong Song, Sergey Senozhatsky, Shuah Khan,
Alexei Starovoitov, Stanislav Fomichev, Daniel Borkmann,
Martin Lau, Song Liu, linux-kselftest, netdev, bpf
On (03/20/19 15:27), Stanislav Fomichev wrote:
[..]
> > Inside kernel clang compiler header (linux/compiler-clang.h) does not
> > define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> > the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.
Oh, hmm, OK. I see your point now. bpf insn set for variables.
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 17:13 ` [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Stanislav Fomichev
2019-03-20 22:20 ` Yonghong Song
@ 2019-03-21 0:22 ` Sergey Senozhatsky
1 sibling, 0 replies; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-21 0:22 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kselftest, netdev, bpf
On (03/20/19 10:13), Stanislav Fomichev wrote:
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.
Thanks!
> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).
Honestly, I haven't, but I think we should be fine.
For !__HAVE_BUILTIN_BSWAP16__ compilers we still do constant folding.
swab16/swab32 turn into
__builtin_constant_p((__u16)(x)) ? ___constant_swab16(x) : __fswab16(x))
and
__builtin_constant_p((__u32)(x)) ? ___constant_swab32(x) : __fswab32(x))
clang/llvm support __builtin_constant_p GCC extension [1]:
: Clang supports a number of builtin library functions with the same
: syntax as GCC, including things like __builtin_nan, __builtin_constant_p,
: __builtin_choose_expr, __builtin_types_compatible_p,
: __builtin_assume_aligned, __sync_fetch_and_add, etc.
So clang should be able to detect swab on a compile time constant and
optimize it.
[1] https://clang.llvm.org/docs/LanguageExtensions.html
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-20 12:53 [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Sergey Senozhatsky
` (2 preceding siblings ...)
2019-03-20 17:13 ` [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection Stanislav Fomichev
@ 2019-03-21 3:24 ` Alexei Starovoitov
2019-03-21 5:08 ` Sergey Senozhatsky
3 siblings, 1 reply; 19+ messages in thread
From: Alexei Starovoitov @ 2019-03-21 3:24 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Shuah Khan, Alexei Starovoitov, Stanislav Fomichev,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kselftest, netdev, bpf
On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
>
> (__builtin_constant_p(x) ? \
> ___constant_swab16(x) : __builtin_bswap16(x))
>
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:
nack to fixes to support such old compilers.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-21 3:24 ` Alexei Starovoitov
@ 2019-03-21 5:08 ` Sergey Senozhatsky
2019-03-21 15:49 ` Stanislav Fomichev
0 siblings, 1 reply; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-21 5:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Sergey Senozhatsky, Shuah Khan, Alexei Starovoitov,
Stanislav Fomichev, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kselftest, netdev, bpf
On (03/20/19 20:24), Alexei Starovoitov wrote:
> On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > thus not all compilers are able to compile the following code:
> >
> > (__builtin_constant_p(x) ? \
> > ___constant_swab16(x) : __builtin_bswap16(x))
> >
> > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > instance:
>
> nack to fixes to support such old compilers.
Fair enough.
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-21 5:08 ` Sergey Senozhatsky
@ 2019-03-21 15:49 ` Stanislav Fomichev
2019-03-22 2:46 ` Sergey Senozhatsky
0 siblings, 1 reply; 19+ messages in thread
From: Stanislav Fomichev @ 2019-03-21 15:49 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Alexei Starovoitov, Sergey Senozhatsky, Shuah Khan,
Alexei Starovoitov, Stanislav Fomichev, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, linux-kselftest,
netdev, bpf
On 03/21, Sergey Senozhatsky wrote:
> On (03/20/19 20:24), Alexei Starovoitov wrote:
> > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > thus not all compilers are able to compile the following code:
> > >
> > > (__builtin_constant_p(x) ? \
> > > ___constant_swab16(x) : __builtin_bswap16(x))
> > >
> > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > instance:
> >
> > nack to fixes to support such old compilers.
>
> Fair enough.
What is too old? Documentation/process/changes.rst says that minimum
supported gcc is 4.6, do we lift that requirement for the tests?
> -ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-21 15:49 ` Stanislav Fomichev
@ 2019-03-22 2:46 ` Sergey Senozhatsky
2019-03-22 3:12 ` Alexei Starovoitov
0 siblings, 1 reply; 19+ messages in thread
From: Sergey Senozhatsky @ 2019-03-22 2:46 UTC (permalink / raw)
To: Stanislav Fomichev, Alexei Starovoitov
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Shuah Khan,
Alexei Starovoitov, Stanislav Fomichev, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, linux-kselftest,
netdev, bpf
On (03/21/19 08:49), Stanislav Fomichev wrote:
> On 03/21, Sergey Senozhatsky wrote:
> > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > thus not all compilers are able to compile the following code:
> > > >
> > > > (__builtin_constant_p(x) ? \
> > > > ___constant_swab16(x) : __builtin_bswap16(x))
> > > >
> > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > instance:
> > >
> > > nack to fixes to support such old compilers.
> >
> > Fair enough.
> What is too old? Documentation/process/changes.rst says that minimum
> supported gcc is 4.6, do we lift that requirement for the tests?
Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
asm goto and hence 4.9 is the minimum supported version. But it seems
that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
means that those bpf defines won't work on some compilers.
Alexei, does your NACK still stand?
-ss
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection
2019-03-22 2:46 ` Sergey Senozhatsky
@ 2019-03-22 3:12 ` Alexei Starovoitov
0 siblings, 0 replies; 19+ messages in thread
From: Alexei Starovoitov @ 2019-03-22 3:12 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Stanislav Fomichev, Sergey Senozhatsky, Shuah Khan,
Alexei Starovoitov, Stanislav Fomichev, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, linux-kselftest,
netdev, bpf
On Fri, Mar 22, 2019 at 11:46:52AM +0900, Sergey Senozhatsky wrote:
> On (03/21/19 08:49), Stanislav Fomichev wrote:
> > On 03/21, Sergey Senozhatsky wrote:
> > > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > > thus not all compilers are able to compile the following code:
> > > > >
> > > > > (__builtin_constant_p(x) ? \
> > > > > ___constant_swab16(x) : __builtin_bswap16(x))
> > > > >
> > > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > > instance:
> > > >
> > > > nack to fixes to support such old compilers.
> > >
> > > Fair enough.
> > What is too old? Documentation/process/changes.rst says that minimum
> > supported gcc is 4.6, do we lift that requirement for the tests?
>
> Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
> asm goto and hence 4.9 is the minimum supported version. But it seems
> that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
> means that those bpf defines won't work on some compilers.
>
> Alexei, does your NACK still stand?
yes.
bpf samples and selftests require llvm and new features like BTF
require the latest llvm which requires gcc 5.1.
Things are more or less working still with gcc 4.8, but soon will
likely start breaking.
^ permalink raw reply [flat|nested] 19+ messages in thread