diff mbox series

[bpf-next,v1,1/4] bpf: allow specifying bpf_fastcall attribute for BPF helpers

Message ID 20240916091712.2929279-2-eddyz87@gmail.com (mailing list archive)
State Accepted
Commit 8143f960d915f3ee38ab065a37cea078bbf46235
Delegated to: BPF
Headers show
Series 'bpf_fastcall' attribute in vmlinux.h and bpf_helper_defs.h | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7 this patch: 7
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 6 maintainers not CCed: song@kernel.org sdf@fomichev.me haoluo@google.com jolsa@kernel.org kpsingh@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 89 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc

Commit Message

Eduard Zingerman Sept. 16, 2024, 9:17 a.m. UTC 
Allow a new optional 'Attributes' section to be specified for helper
functions description, e.g.:

 * u32 bpf_get_smp_processor_id(void)
 * 		...
 * 	Return
 * 		...
 * 	Attributes
 * 		__bpf_fastcall
 *

Generated header for the example above:

  #ifndef __bpf_fastcall
  #if __has_attribute(__bpf_fastcall)
  #define __bpf_fastcall __attribute__((bpf_fastcall))
  #else
  #define __bpf_fastcall
  #endif
  #endif
  ...
  __bpf_fastcall
  static __u32 (* const bpf_get_smp_processor_id)(void) = (void *) 8;

The following rules apply:
- when present, section must follow 'Return' section;
- attribute names are specified on the line following 'Attribute'
  keyword;
- attribute names are separated by spaces;
- section ends with an "empty" line (" *\n").

Valid attribute names are recorded in the ATTRS map.
ATTRS maps shortcut attribute name to correct C syntax.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 scripts/bpf_doc.py | 50 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Sept. 27, 2024, 10:31 p.m. UTC | #1 
On Mon, Sep 16, 2024 at 2:18 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Allow a new optional 'Attributes' section to be specified for helper
> functions description, e.g.:
>
>  * u32 bpf_get_smp_processor_id(void)
>  *              ...
>  *      Return
>  *              ...
>  *      Attributes
>  *              __bpf_fastcall
>  *
>
> Generated header for the example above:
>
>   #ifndef __bpf_fastcall
>   #if __has_attribute(__bpf_fastcall)
>   #define __bpf_fastcall __attribute__((bpf_fastcall))
>   #else
>   #define __bpf_fastcall
>   #endif
>   #endif
>   ...
>   __bpf_fastcall

I found it a bit annoying that bpf_helper_defs.h uses

__bpf_fastcall
static __u32 ....

format, while in vmlinux we have single-line (and yeah, note that I
put extern in front)

extern __bpf_fastcall __u32 ...


So I slightly modified bpf_doc.py with my weak Python-fu:

diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index db50c8d7d112..f98933a5d38c 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -871,9 +871,10 @@ class PrinterHelpers(Printer):
                 print(' *{}{}'.format(' \t' if line else '', line))

         print(' */')
+        print('static ', end='')
         if helper.attrs:
-            print(" ".join(helper.attrs))
-        print('static %s %s(* const %s)(' % (self.map_type(proto['ret_type']),
+            print('%s ' % (" ".join(helper.attrs)), end='')
+        print('%s %s(* const %s)(' % (self.map_type(proto['ret_type']),
                                       proto['ret_star'],
proto['name']), end='')
         comma = ''
         for i, a in enumerate(proto['args']):

But now I have:

static __bpf_fastcall __u32 (* const bpf_get_smp_processor_id)(void) =
(void *) 8;

and

extern __bpf_fastcall void *bpf_rdonly_cast(const void *obj__ign, u32
btf_id__k) __weak __ksym;

and that makes me a touch happier. I hope you don't mind.

>   static __u32 (* const bpf_get_smp_processor_id)(void) = (void *) 8;
>
> The following rules apply:
> - when present, section must follow 'Return' section;
> - attribute names are specified on the line following 'Attribute'
>   keyword;
> - attribute names are separated by spaces;
> - section ends with an "empty" line (" *\n").
>
> Valid attribute names are recorded in the ATTRS map.
> ATTRS maps shortcut attribute name to correct C syntax.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  scripts/bpf_doc.py | 50 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
>

Looks good to me, and I'm not sure there is anything too controversial
here, so I went ahead and applied to bpf-next, thanks.
Eduard Zingerman Sept. 27, 2024, 10:39 p.m. UTC | #2 
On Fri, 2024-09-27 at 15:31 -0700, Andrii Nakryiko wrote:

[...]

> @@ -871,9 +871,10 @@ class PrinterHelpers(Printer):
>                  print(' *{}{}'.format(' \t' if line else '', line))
> 
>          print(' */')
> +        print('static ', end='')
>          if helper.attrs:
> -            print(" ".join(helper.attrs))
> -        print('static %s %s(* const %s)(' % (self.map_type(proto['ret_type']),
> +            print('%s ' % (" ".join(helper.attrs)), end='')
> +        print('%s %s(* const %s)(' % (self.map_type(proto['ret_type']),
>                                        proto['ret_star'],
> proto['name']), end='')
>          comma = ''
>          for i, a in enumerate(proto['args']):
> 
> But now I have:
> 
> static __bpf_fastcall __u32 (* const bpf_get_smp_processor_id)(void) =
> (void *) 8;
> 
> and
> 
> extern __bpf_fastcall void *bpf_rdonly_cast(const void *obj__ign, u32
> btf_id__k) __weak __ksym;
> 
> and that makes me a touch happier. I hope you don't mind.

This change looks fine to me.

[...]

> Looks good to me, and I'm not sure there is anything too controversial
> here, so I went ahead and applied to bpf-next, thanks.

Great, thank you.
diff mbox series

Patch

diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index c55878bddfdd..db50c8d7d112 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -37,10 +37,11 @@  class APIElement(object):
     @desc: textual description of the symbol
     @ret: (optional) description of any associated return value
     """
-    def __init__(self, proto='', desc='', ret=''):
+    def __init__(self, proto='', desc='', ret='', attrs=[]):
         self.proto = proto
         self.desc = desc
         self.ret = ret
+        self.attrs = attrs
 
 
 class Helper(APIElement):
@@ -81,6 +82,11 @@  class Helper(APIElement):
         return res
 
 
+ATTRS = {
+    '__bpf_fastcall': 'bpf_fastcall'
+}
+
+
 class HeaderParser(object):
     """
     An object used to parse a file in order to extract the documentation of a
@@ -111,7 +117,8 @@  class HeaderParser(object):
         proto    = self.parse_proto()
         desc     = self.parse_desc(proto)
         ret      = self.parse_ret(proto)
-        return Helper(proto=proto, desc=desc, ret=ret)
+        attrs    = self.parse_attrs(proto)
+        return Helper(proto=proto, desc=desc, ret=ret, attrs=attrs)
 
     def parse_symbol(self):
         p = re.compile(r' \* ?(BPF\w+)$')
@@ -192,6 +199,28 @@  class HeaderParser(object):
             raise Exception("No return found for " + proto)
         return ret
 
+    def parse_attrs(self, proto):
+        p = re.compile(r' \* ?(?:\t| {5,8})Attributes$')
+        capture = p.match(self.line)
+        if not capture:
+            return []
+        # Expect a single line with mnemonics for attributes separated by spaces
+        self.line = self.reader.readline()
+        p = re.compile(r' \* ?(?:\t| {5,8})(?:\t| {8})(.*)')
+        capture = p.match(self.line)
+        if not capture:
+            raise Exception("Incomplete 'Attributes' section for " + proto)
+        attrs = capture.group(1).split(' ')
+        for attr in attrs:
+            if attr not in ATTRS:
+                raise Exception("Unexpected attribute '" + attr + "' specified for " + proto)
+        self.line = self.reader.readline()
+        if self.line != ' *\n':
+            raise Exception("Expecting empty line after 'Attributes' section for " + proto)
+        # Prepare a line for next self.parse_* to consume
+        self.line = self.reader.readline()
+        return attrs
+
     def seek_to(self, target, help_message, discard_lines = 1):
         self.reader.seek(0)
         offset = self.reader.read().find(target)
@@ -789,6 +818,21 @@  class PrinterHelpers(Printer):
             print('%s;' % fwd)
         print('')
 
+        used_attrs = set()
+        for helper in self.elements:
+            for attr in helper.attrs:
+                used_attrs.add(attr)
+        for attr in sorted(used_attrs):
+            print('#ifndef %s' % attr)
+            print('#if __has_attribute(%s)' % ATTRS[attr])
+            print('#define %s __attribute__((%s))' % (attr, ATTRS[attr]))
+            print('#else')
+            print('#define %s' % attr)
+            print('#endif')
+            print('#endif')
+        if used_attrs:
+            print('')
+
     def print_footer(self):
         footer = ''
         print(footer)
@@ -827,6 +871,8 @@  class PrinterHelpers(Printer):
                 print(' *{}{}'.format(' \t' if line else '', line))
 
         print(' */')
+        if helper.attrs:
+            print(" ".join(helper.attrs))
         print('static %s %s(* const %s)(' % (self.map_type(proto['ret_type']),
                                       proto['ret_star'], proto['name']), end='')
         comma = ''