diff mbox series

[v3] bpf.2: Use standard types and attributes

Message ID 20210515190116.188362-1-alx.manpages@gmail.com (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series [v3] bpf.2: Use standard types and attributes | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Alejandro Colomar May 15, 2021, 7:01 p.m. UTC
Some manual pages are already using C99 syntax for integral
types 'uint32_t', but some aren't.  There are some using kernel
syntax '__u32'.  Fix those.

Both the kernel and the standard types are 100% binary compatible,
and the source code differences between them are very small, and
not important in a manual page:

- Some of them are implemented with different underlying types
  (e.g., s64 is always long long, while int64_t may be long long
  or long, depending on the arch).  This causes the following
  differences.

- length modifiers required by printf are different, resulting in
  a warning ('-Wformat=').

- pointer assignment causes a warning:
  ('-Wincompatible-pointer-types'), but there aren't any pointers
  in this page.

But, AFAIK, all of those warnings can be safely ignored, due to
the binary compatibility between the types.

...

Some pages also document attributes, using GNU syntax
'__attribute__((xxx))'.  Update those to use the shorter and more
portable C11 keywords such as 'alignas()' when possible, and C2x
syntax '[[gnu::xxx]]' elsewhere, which hasn't been standardized
yet, but is already implemented in GCC, and available through
either --std=c2x or any of the --std=gnu... options.

The standard isn't very clear on how to use alignas() or
[[]]-style attributes, and the GNU documentation isn't better, so
the following link is a useful experiment about the different
alignment syntaxes:
__attribute__((aligned())), alignas(), and [[gnu::aligned()]]:
<https://stackoverflow.com/q/67271825/6872717>

Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
Nacked-by: Alexei Starovoitov <ast@kernel.org>
Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Zack Weinberg <zackw@panix.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: glibc <libc-alpha@sourceware.org>
Cc: GCC <gcc-patches@gcc.gnu.org>
Cc: bpf <bpf@vger.kernel.org>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 man2/bpf.2 | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

Comments

Alejandro Colomar May 16, 2021, 9:16 a.m. UTC | #1
On 5/15/21 9:01 PM, Alejandro Colomar wrote:
> Some manual pages are already using C99 syntax for integral
> types 'uint32_t', but some aren't.  There are some using kernel
> syntax '__u32'.  Fix those.
> 
> Both the kernel and the standard types are 100% binary compatible,
> and the source code differences between them are very small, and
> not important in a manual page:
> 
> - Some of them are implemented with different underlying types
>    (e.g., s64 is always long long, while int64_t may be long long
>    or long, depending on the arch).  This causes the following
>    differences.
> 
> - length modifiers required by printf are different, resulting in
>    a warning ('-Wformat=').
> 
> - pointer assignment causes a warning:
>    ('-Wincompatible-pointer-types'), but there aren't any pointers
>    in this page.
> 
> But, AFAIK, all of those warnings can be safely ignored, due to
> the binary compatibility between the types.
> 
> ...
> 
> Some pages also document attributes, using GNU syntax
> '__attribute__((xxx))'.  Update those to use the shorter and more
> portable C11 keywords such as 'alignas()' when possible, and C2x
> syntax '[[gnu::xxx]]' elsewhere, which hasn't been standardized
> yet, but is already implemented in GCC, and available through
> either --std=c2x or any of the --std=gnu... options.
> 
> The standard isn't very clear on how to use alignas() or
> [[]]-style attributes, and the GNU documentation isn't better, so
> the following link is a useful experiment about the different
> alignment syntaxes:
> __attribute__((aligned())), alignas(), and [[gnu::aligned()]]:
> <https://stackoverflow.com/q/67271825/6872717>
> 
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
Discussion: 
<https://lore.kernel.org/linux-man/6740a229-842e-b368-86eb-defc786b3658@gmail.com/T/>
> Nacked-by: Alexei Starovoitov <ast@kernel.org>
> Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Zack Weinberg <zackw@panix.com>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: glibc <libc-alpha@sourceware.org>
> Cc: GCC <gcc-patches@gcc.gnu.org>
> Cc: bpf <bpf@vger.kernel.org>
> Cc: David Laight <David.Laight@ACULAB.COM>
> Cc: Joseph Myers <joseph@codesourcery.com>
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> ---
>   man2/bpf.2 | 49 ++++++++++++++++++++++++-------------------------
>   1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/man2/bpf.2 b/man2/bpf.2
> index 6e1ffa198..04b8fbcef 100644
> --- a/man2/bpf.2
> +++ b/man2/bpf.2
> @@ -186,41 +186,40 @@ commands:
>   .PP
>   .in +4n
>   .EX
> -union bpf_attr {
> +union [[gnu::aligned(8)]] bpf_attr {
>       struct {    /* Used by BPF_MAP_CREATE */
> -        __u32         map_type;
> -        __u32         key_size;    /* size of key in bytes */
> -        __u32         value_size;  /* size of value in bytes */
> -        __u32         max_entries; /* maximum number of entries
> -                                      in a map */
> +        uint32_t    map_type;
> +        uint32_t    key_size;    /* size of key in bytes */
> +        uint32_t    value_size;  /* size of value in bytes */
> +        uint32_t    max_entries; /* maximum number of entries
> +                                    in a map */
>       };
>   
> -    struct {    /* Used by BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY
> -                   commands */
> -        __u32         map_fd;
> -        __aligned_u64 key;
> +    struct {    /* Used by BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY commands */
> +        uint32_t            map_fd;
> +        uint64_t alignas(8) key;
>           union {
> -            __aligned_u64 value;
> -            __aligned_u64 next_key;
> +            uint64_t alignas(8) value;
> +            uint64_t alignas(8) next_key;
>           };
> -        __u64         flags;
> +        uint64_t            flags;
>       };
>   
>       struct {    /* Used by BPF_PROG_LOAD */
> -        __u32         prog_type;
> -        __u32         insn_cnt;
> -        __aligned_u64 insns;      /* \(aqconst struct bpf_insn *\(aq */
> -        __aligned_u64 license;    /* \(aqconst char *\(aq */
> -        __u32         log_level;  /* verbosity level of verifier */
> -        __u32         log_size;   /* size of user buffer */
> -        __aligned_u64 log_buf;    /* user supplied \(aqchar *\(aq
> -                                     buffer */
> -        __u32         kern_version;
> -                                  /* checked when prog_type=kprobe
> -                                     (since Linux 4.1) */
> +        uint32_t            prog_type;
> +        uint32_t            insn_cnt;
> +        uint64_t alignas(8) insns;     /* \(aqconst struct bpf_insn *\(aq */
> +        uint64_t alignas(8) license;   /* \(aqconst char *\(aq */
> +        uint32_t            log_level; /* verbosity level of verifier */
> +        uint32_t            log_size;  /* size of user buffer */
> +        uint64_t alignas(8) log_buf;   /* user supplied \(aqchar *\(aq
> +                                          buffer */
> +        uint32_t            kern_version;
> +                                       /* checked when prog_type=kprobe
> +                                          (since Linux 4.1) */
>   .\"                 commit 2541517c32be2531e0da59dfd7efc1ce844644f5
>       };
> -} __attribute__((aligned(8)));
> +};
>   .EE
>   .in
>   .\"
>
Daniel Borkmann May 17, 2021, 6:56 p.m. UTC | #2
On 5/16/21 11:16 AM, Alejandro Colomar (man-pages) wrote:
> On 5/15/21 9:01 PM, Alejandro Colomar wrote:
>> Some manual pages are already using C99 syntax for integral
>> types 'uint32_t', but some aren't.  There are some using kernel
>> syntax '__u32'.  Fix those.
>>
>> Both the kernel and the standard types are 100% binary compatible,
>> and the source code differences between them are very small, and
>> not important in a manual page:
>>
>> - Some of them are implemented with different underlying types
>>    (e.g., s64 is always long long, while int64_t may be long long
>>    or long, depending on the arch).  This causes the following
>>    differences.
>>
>> - length modifiers required by printf are different, resulting in
>>    a warning ('-Wformat=').
>>
>> - pointer assignment causes a warning:
>>    ('-Wincompatible-pointer-types'), but there aren't any pointers
>>    in this page.
>>
>> But, AFAIK, all of those warnings can be safely ignored, due to
>> the binary compatibility between the types.
>>
>> ...
>>
>> Some pages also document attributes, using GNU syntax
>> '__attribute__((xxx))'.  Update those to use the shorter and more
>> portable C11 keywords such as 'alignas()' when possible, and C2x
>> syntax '[[gnu::xxx]]' elsewhere, which hasn't been standardized
>> yet, but is already implemented in GCC, and available through
>> either --std=c2x or any of the --std=gnu... options.
>>
>> The standard isn't very clear on how to use alignas() or
>> [[]]-style attributes, and the GNU documentation isn't better, so
>> the following link is a useful experiment about the different
>> alignment syntaxes:
>> __attribute__((aligned())), alignas(), and [[gnu::aligned()]]:
>> <https://stackoverflow.com/q/67271825/6872717>
>>
>> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> Discussion: <https://lore.kernel.org/linux-man/6740a229-842e-b368-86eb-defc786b3658@gmail.com/T/>
>> Nacked-by: Alexei Starovoitov <ast@kernel.org>
>> Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

You forgot to retain my ...

Nacked-by: Daniel Borkmann <daniel@iogearbox.net>

>> Acked-by: Zack Weinberg <zackw@panix.com>
>> Cc: LKML <linux-kernel@vger.kernel.org>
>> Cc: glibc <libc-alpha@sourceware.org>
>> Cc: GCC <gcc-patches@gcc.gnu.org>
>> Cc: bpf <bpf@vger.kernel.org>
>> Cc: David Laight <David.Laight@ACULAB.COM>
>> Cc: Joseph Myers <joseph@codesourcery.com>
>> Cc: Florian Weimer <fweimer@redhat.com>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   man2/bpf.2 | 49 ++++++++++++++++++++++++-------------------------
>>   1 file changed, 24 insertions(+), 25 deletions(-)
>>
Alejandro Colomar May 21, 2021, 11:12 a.m. UTC | #3
Hello Daniel,

On 5/17/21 8:56 PM, Daniel Borkmann wrote:
> On 5/16/21 11:16 AM, Alejandro Colomar (man-pages) wrote:
>>>
>>> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
>> Discussion: 
>> <https://lore.kernel.org/linux-man/6740a229-842e-b368-86eb-defc786b3658@gmail.com/T/> 
>>
>>> Nacked-by: Alexei Starovoitov <ast@kernel.org>
>>> Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> You forgot to retain my ...
> 
> Nacked-by: Daniel Borkmann <daniel@iogearbox.net>

Yup! Sorry, I forgot :)

Thanks,

Alex

> 
>>> Acked-by: Zack Weinberg <zackw@panix.com>
>>> Cc: LKML <linux-kernel@vger.kernel.org>
>>> Cc: glibc <libc-alpha@sourceware.org>
>>> Cc: GCC <gcc-patches@gcc.gnu.org>
>>> Cc: bpf <bpf@vger.kernel.org>
>>> Cc: David Laight <David.Laight@ACULAB.COM>
>>> Cc: Joseph Myers <joseph@codesourcery.com>
>>> Cc: Florian Weimer <fweimer@redhat.com>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> ---
>>>   man2/bpf.2 | 49 ++++++++++++++++++++++++-------------------------
>>>   1 file changed, 24 insertions(+), 25 deletions(-)
>>>
diff mbox series

Patch

diff --git a/man2/bpf.2 b/man2/bpf.2
index 6e1ffa198..04b8fbcef 100644
--- a/man2/bpf.2
+++ b/man2/bpf.2
@@ -186,41 +186,40 @@  commands:
 .PP
 .in +4n
 .EX
-union bpf_attr {
+union [[gnu::aligned(8)]] bpf_attr {
     struct {    /* Used by BPF_MAP_CREATE */
-        __u32         map_type;
-        __u32         key_size;    /* size of key in bytes */
-        __u32         value_size;  /* size of value in bytes */
-        __u32         max_entries; /* maximum number of entries
-                                      in a map */
+        uint32_t    map_type;
+        uint32_t    key_size;    /* size of key in bytes */
+        uint32_t    value_size;  /* size of value in bytes */
+        uint32_t    max_entries; /* maximum number of entries
+                                    in a map */
     };
 
-    struct {    /* Used by BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY
-                   commands */
-        __u32         map_fd;
-        __aligned_u64 key;
+    struct {    /* Used by BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY commands */
+        uint32_t            map_fd;
+        uint64_t alignas(8) key;
         union {
-            __aligned_u64 value;
-            __aligned_u64 next_key;
+            uint64_t alignas(8) value;
+            uint64_t alignas(8) next_key;
         };
-        __u64         flags;
+        uint64_t            flags;
     };
 
     struct {    /* Used by BPF_PROG_LOAD */
-        __u32         prog_type;
-        __u32         insn_cnt;
-        __aligned_u64 insns;      /* \(aqconst struct bpf_insn *\(aq */
-        __aligned_u64 license;    /* \(aqconst char *\(aq */
-        __u32         log_level;  /* verbosity level of verifier */
-        __u32         log_size;   /* size of user buffer */
-        __aligned_u64 log_buf;    /* user supplied \(aqchar *\(aq
-                                     buffer */
-        __u32         kern_version;
-                                  /* checked when prog_type=kprobe
-                                     (since Linux 4.1) */
+        uint32_t            prog_type;
+        uint32_t            insn_cnt;
+        uint64_t alignas(8) insns;     /* \(aqconst struct bpf_insn *\(aq */
+        uint64_t alignas(8) license;   /* \(aqconst char *\(aq */
+        uint32_t            log_level; /* verbosity level of verifier */
+        uint32_t            log_size;  /* size of user buffer */
+        uint64_t alignas(8) log_buf;   /* user supplied \(aqchar *\(aq
+                                          buffer */
+        uint32_t            kern_version;
+                                       /* checked when prog_type=kprobe
+                                          (since Linux 4.1) */
 .\"                 commit 2541517c32be2531e0da59dfd7efc1ce844644f5
     };
-} __attribute__((aligned(8)));
+};
 .EE
 .in
 .\"