Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




在 2022/10/10 16:40, Quentin Monnet 写道:
Sat Oct 08 2022 06:16:42 GMT+0100 ~ wangyufen <wangyufen@xxxxxxxxxx>
在 2022/10/1 0:26, Quentin Monnet 写道:
Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@xxxxxxxxxx>
Add auto_attach optional to support one-step load-attach-pin_link.
Nit: Now "autoattach" instead of "auto_attach". Same in commit title.
will change in v8, thanks.
For example,
     $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach

     $ bpftool link
     26: tracing  name test1  tag f0da7d0058c00236  gpl
         loaded_at 2022-09-09T21:39:49+0800  uid 0
         xlated 88B  jited 55B  memlock 4096B  map_ids 3
         btf_id 55
     28: kprobe  name test3  tag 002ef1bef0723833  gpl
         loaded_at 2022-09-09T21:39:49+0800  uid 0
         xlated 88B  jited 56B  memlock 4096B  map_ids 3
         btf_id 55
     57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
         loaded_at 2022-09-09T21:41:32+0800  uid 0
         xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
         btf_id 82

     $ bpftool link
     1: tracing  prog 26
         prog_type tracing  attach_type trace_fentry
     3: perf_event  prog 28
     10: perf_event  prog 57

The autoattach optional can support tracepoints, k(ret)probes,
u(ret)probes.

Signed-off-by: Wei Yongjun <weiyongjun1@xxxxxxxxxx>
Signed-off-by: Wang Yufen <wangyufen@xxxxxxxxxx>
---
v6 -> v7: add info msg print and update doc for the skip program
v5 -> v6: skip the programs not supporting auto-attach,
       and change optional name from "auto_attach" to "autoattach"
v4 -> v5: some formatting nits of doc
v3 -> v4: rename functions, update doc, bash and do_help()
v2 -> v3: switch to extend prog load command instead of extend perf
v2:
https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@xxxxxxxxxx/
v1:
https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@xxxxxxxxxx/
   tools/bpf/bpftool/prog.c | 81
++++++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index c81362a..84eced8 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
       return ret;
   }
   +static int
+auto_attach_program(struct bpf_program *prog, const char *path)
+{
+    struct bpf_link *link;
+    int err;
+
+    link = bpf_program__attach(prog);
+    if (!link)
+        return -1;
+
+    err = bpf_link__pin(link, path);
+    if (err) {
+        bpf_link__destroy(link);
+        return err;
+    }
+    return 0;
+}
+
+static int pathname_concat(const char *path, const char *name, char
*buf)
+{
+    int len;
+
+    len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
+    if (len < 0)
+        return -EINVAL;
+    if (len >= PATH_MAX)
+        return -ENAMETOOLONG;
+
+    return 0;
+}
+
+static int
+auto_attach_programs(struct bpf_object *obj, const char *path)
+{
+    struct bpf_program *prog;
+    char buf[PATH_MAX];
+    int err;
+
+    bpf_object__for_each_program(prog, obj) {
+        err = pathname_concat(path, bpf_program__name(prog), buf);
+        if (err)
+            goto err_unpin_programs;
+
+        err = auto_attach_program(prog, buf);
+        if (!err)
+            continue;
+        if (errno == EOPNOTSUPP)
+            p_info("Program %s does not support autoattach",
+                   bpf_program__name(prog));
+        else
+            goto err_unpin_programs
With this code, if auto-attach fails, then we skip this program and move
on to the next. That's an improvement, but in that case the program
won't remain loaded in the kernel after bpftool exits. My suggestion in
my previous message (sorry if it was not clear) was to fall back to
regular pinning in that case (bpf_obj_pin()), along with the p_info()
message, so we can have the program pinned but not attached and let the
user know. If regular pinning fails as well, then we should unpin all
and error out, for consistency with bpf_object__pin_programs().

And in that case, the (errno == EOPNOTSUPP) with fallback to regular
pinning could maybe be moved into auto_attach_program(), so that
auto-attaching single programs can use the fallback too?

Thanks,
Quentin
If I understand correctly, can we just check link?  as following:
Yes, this is exactly what I meant

--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1460,9 +1460,10 @@ static int do_run(int argc, char **argv)
         int err;
        link = bpf_program__attach(prog);
-       if (!link)
-               return -1;
-
+       if (!link) {
+               p_info("Program %s attach failed",
bpf_program__name(prog));
+               return bpf_obj_pin(bpf_program__fd(prog), path);
+       }
         err = bpf_link__pin(link, path);
         if (err) {
                 bpf_link__destroy(link);
@@ -1499,9 +1500,6 @@ static int pathname_concat(const char *path, const
char *name, char *buf)
                 err = auto_attach_program(prog, buf);
                 if (!err)
                         continue;
-               if (errno == EOPNOTSUPP)
-                       p_info("Program %s does not support autoattach",
p_info("Program %s does not support autoattach, falling back to pinning"

-                              bpf_program__name(prog));
                 else
                         goto err_unpin_programs;
         }


and the doc is modified as follows:

If the program does not support autoattach, will do regular pin along
with an
info message such as "Program %s attach failed". If the *OBJ* contains
multiple
programs and **loadall** is used, if the program A in these programs
does not
support autoattach, the program A will do regular pin along with an info
message,
and continue to autoattach the next program.
Not sure the "program A" designation helps too much, I'd simply write this:

"If a program does not support autoattach, bpftool falls back to regular
pinning for that program instead."

Which should be enough for both the "load" and "loadall" behaviours? I
wouldn't mention the help message in the docs (the p_info() won't show
up in the JSON output for example).

I got it. Thanks!


Looks good otherwise, thanks!



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux