Re: [RFC] bpf.2: Use standard types and attributes
Hi Joseph, On 4/26/21 7:19 PM, Joseph Myers wrote: On Sat, 24 Apr 2021, Alejandro Colomar via Libc-alpha wrote: Some pages also document attributes, using GNU syntax '__attribute__((xxx))'. Update those to use the shorter and more portable C2x syntax, 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. If you mention alignment in the manpage at all, the same reasoning would say you should use _Alignas(8) not [[gnu::aligned(8)]], in any context where _Alignas is valid. Agree. I just didn't know 'alignas()' (a.k.a. '_Alignas()'), so I used attributes and only changed the syntax. But yes, we should use that C11 feature. Given that we already used 'noreturn' and not '_Noreturn' (see exit(3) and its family), I'll use 'alignas()'. I'll send a v2 with those changes. Thanks, Alex -- Alejandro Colomar Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/ http://www.alejandro-colomar.es/
Re: [RFC] bpf.2: Use standard types and attributes
On Sat, 24 Apr 2021, Alejandro Colomar via Libc-alpha wrote: > Some pages also document attributes, using GNU syntax > '__attribute__((xxx))'. Update those to use the shorter and more > portable C2x syntax, 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. If you mention alignment in the manpage at all, the same reasoning would say you should use _Alignas(8) not [[gnu::aligned(8)]], in any context where _Alignas is valid. -- Joseph S. Myers jos...@codesourcery.com
RE: [RFC] bpf.2: Use standard types and attributes
From: Zack Weinberg > Sent: 25 April 2021 20:17 > > On Sat, Apr 24, 2021 at 4:43 PM David Laight via Libc-alpha > wrote: > > From: Alexei Starovoitov > > > On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar > > > wrote: > ... > > > > Some pages also document attributes, using GNU syntax > > > > '__attribute__((xxx))'. Update those to use the shorter and more > > > > portable C2x syntax, 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. > .. > > And the code below is no more portable that a #pragma'. > > It is probably worse than __attribute__((aligned(8))) > > +uint64_t [[gnu::aligned(8)]] value; > > The standards committee are smoking dope again. > > At least the '__aligned_u64 value;' form stands a reasonable > > chance of being converted by cpp into whatever your compiler supports. > > Is it actually necessary to mention the alignment overrides at all in > the manpages? They are only relevant to people working at the level > of physical layout of the data in RAM, and those people are probably > going to have to consult the header file anyway. Depends, if the man page defines the structure - it needs to contain its definition. If theory the man page ought to be the definition, and the code do what the man page says happens. An alternative is for the man page to say that the structure contains some fields - without prescribing the order, or stopping the implementation adding additional fields (or even changing the actual numeric type). This is more common in the standards documents. IMHO The Linux pages really ought to say how linux does things. (With notes about portability.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [RFC] bpf.2: Use standard types and attributes
On Sat, Apr 24, 2021 at 4:43 PM David Laight via Libc-alpha wrote: > From: Alexei Starovoitov > > On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar > > wrote: ... > > > Some pages also document attributes, using GNU syntax > > > '__attribute__((xxx))'. Update those to use the shorter and more > > > portable C2x syntax, 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. .. > And the code below is no more portable that a #pragma'. > It is probably worse than __attribute__((aligned(8))) > +uint64_t [[gnu::aligned(8)]] value; > The standards committee are smoking dope again. > At least the '__aligned_u64 value;' form stands a reasonable > chance of being converted by cpp into whatever your compiler supports. Is it actually necessary to mention the alignment overrides at all in the manpages? They are only relevant to people working at the level of physical layout of the data in RAM, and those people are probably going to have to consult the header file anyway. zw
Re: [RFC] bpf.2: Use standard types and attributes
On Sun, Apr 25, 2021 at 12:52 PM Alexei Starovoitov via Libc-alpha wrote: > On Sat, Apr 24, 2021 at 10:56 AM Alejandro Colomar (man-pages) > wrote: > > > > Hello Alexei, > > > > On 4/24/21 1:20 AM, Alexei Starovoitov wrote: > > > Nack. > > > The man page should describe the kernel api the way it is in .h file. > > > > Why? > > Because man page must describe the linux uapi headers the way they > are installed in the system and not invent alternative implementations. > The users will include those .h with __u32 and will see them in their code. > Man page saying something else is a dangerous lie. Why do you consider it _dangerous_ for the manpages to replace __u32 with uint32_t, when we know by construction that the two types will always be the same? Alejandro's preference for the types standardized by ISO C seems perfectly reasonable to me for documentation; people reading the documentation can be expected to already know what they mean, unlike the Linux-specifc __[iu]NN types. Also, all else being equal, documentation should avoid use of symbols in the ISO C reserved namespace. If anything I would argue that it is the uapi headers that should be changed, to use the types. zw
Re: [RFC] bpf.2: Use standard types and attributes
On Sat, Apr 24, 2021 at 10:56 AM Alejandro Colomar (man-pages) wrote: > > Hello Alexei, > > On 4/24/21 1:20 AM, Alexei Starovoitov wrote: > > Nack. > > The man page should describe the kernel api the way it is in .h file. > > Why? Because man page must describe the linux uapi headers the way they are installed in the system and not invent alternative implementations. The users will include those .h with __u32 and will see them in their code. Man page saying something else is a dangerous lie. > using uint32_t in every situation where __u32 is expected. They're both > typedefs for the same basic type. That's irrelevant. Languages like golang have their own bpf.h equivalent that matches /usr/include/linux/bpf.h. > I can understand why Linux will keep using u32 types (and their __ user > space variants), but that doesn't mean user space programs need to use > the same type. No one says that the users must use __u32. See golang example. But if the users do #include they will get them and man page must describe that. > If we have a standard syntax for fixed-width integral types (and for > anything, actually), the manual pages should probably follow it, > whenever possible. Absolutely not. linux man page must describe linux.
RE: [RFC] bpf.2: Use standard types and attributes
From: Alexei Starovoitov > Sent: 24 April 2021 00:20 > > On Fri, Apr 23, 2021 at 4:15 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. > > > > Some pages also document attributes, using GNU syntax > > '__attribute__((xxx))'. Update those to use the shorter and more > > portable C2x syntax, 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. > > > > Signed-off-by: Alejandro Colomar > > --- > > man2/bpf.2 | 47 +++ > > 1 file changed, 23 insertions(+), 24 deletions(-) > > > > diff --git a/man2/bpf.2 b/man2/bpf.2 > > index 6e1ffa198..204f01bfc 100644 > > --- a/man2/bpf.2 > > +++ b/man2/bpf.2 > > @@ -188,39 +188,38 @@ commands: > > .EX > > union 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_tmap_type; > > +uint32_tkey_size;/* size of key in bytes */ > > +uint32_tvalue_size; /* size of value in bytes */ > > +uint32_tmax_entries; /* maximum number of entries > > +in a map */ > > Nack. > The man page should describe the kernel api the way it is in .h file. And the code below is no more portable that a #pragma'. It is probably worse than __attribute__((aligned(8))) +uint64_t [[gnu::aligned(8)]] value; The standards committee are smoking dope again. At least the '__aligned_u64 value;' form stands a reasonable chance of being converted by cpp into whatever your compiler supports. OTOH the bfp developers want shooting for defining a structure with hidden padding fields. It they ensured that all 64bit fields were aligned they wouldn't need the __aligned_u64 at all. And would be much less likely to leak kernel stack to userspace. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [RFC] bpf.2: Use standard types and attributes
Hello Alexei, On 4/24/21 1:20 AM, Alexei Starovoitov wrote: Nack. The man page should describe the kernel api the way it is in .h file. Why? When glibc uses __size_t (or any other non-standard types) just because the standard doesn't allow it to define some types in some specific header, the manual pages document the equivalent standard type, (i.e., if glibc uses __size_t, we document size_t). The compiler, AFAIK (gcc is CCd, so they can jump in if I'm wrong), using uint32_t in every situation where __u32 is expected. They're both typedefs for the same basic type. I can understand why Linux will keep using u32 types (and their __ user space variants), but that doesn't mean user space programs need to use the same type. If we have a standard syntax for fixed-width integral types (and for anything, actually), the manual pages should probably follow it, whenever possible. Any deviation from the standard (be it C or POSIX) should have a very good reason to be; otherwise, it only creates confusion. Thanks, Alex -- Alejandro Colomar Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/ Senior SW Engineer; http://www.alejandro-colomar.es/
Re: [RFC] bpf.2: Use standard types and attributes
On Fri, Apr 23, 2021 at 4:15 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. > > Some pages also document attributes, using GNU syntax > '__attribute__((xxx))'. Update those to use the shorter and more > portable C2x syntax, 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. > > Signed-off-by: Alejandro Colomar > --- > man2/bpf.2 | 47 +++ > 1 file changed, 23 insertions(+), 24 deletions(-) > > diff --git a/man2/bpf.2 b/man2/bpf.2 > index 6e1ffa198..204f01bfc 100644 > --- a/man2/bpf.2 > +++ b/man2/bpf.2 > @@ -188,39 +188,38 @@ commands: > .EX > union 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_tmap_type; > +uint32_tkey_size;/* size of key in bytes */ > +uint32_tvalue_size; /* size of value in bytes */ > +uint32_tmax_entries; /* maximum number of entries > +in a map */ Nack. The man page should describe the kernel api the way it is in .h file.
[RFC] bpf.2: Use standard types and attributes
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. Some pages also document attributes, using GNU syntax '__attribute__((xxx))'. Update those to use the shorter and more portable C2x syntax, 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. Signed-off-by: Alejandro Colomar --- man2/bpf.2 | 47 +++ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/man2/bpf.2 b/man2/bpf.2 index 6e1ffa198..204f01bfc 100644 --- a/man2/bpf.2 +++ b/man2/bpf.2 @@ -188,39 +188,38 @@ commands: .EX union 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_tmap_type; +uint32_tkey_size;/* size of key in bytes */ +uint32_tvalue_size; /* size of value in bytes */ +uint32_tmax_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 [[gnu::aligned(8)]] key; union { -__aligned_u64 value; -__aligned_u64 next_key; +uint64_t [[gnu::aligned(8)]] value; +uint64_t [[gnu::aligned(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 [[gnu::aligned(8)]] insns; /* \(aqconst struct bpf_insn *\(aq */ +uint64_t [[gnu::aligned(8)]] license; /* \(aqconst char *\(aq */ +uint32_t log_level; /* verbosity level of verifier */ +uint32_t log_size; /* size of user buffer */ +uint64_t [[gnu::aligned(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))); +} [[gnu::aligned(8)]]; .EE .in .\" -- 2.31.0