| Message ID | 1664277676-2228-1-git-send-email-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v7,1/3] bpftool: Add auto_attach for bpf prog load|loadall | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 17 of 17 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| 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: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 117 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | fail | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com> > Add auto_attach optional to support one-step load-attach-pin_link. Nit: Now "autoattach" instead of "auto_attach". Same in commit title. > > 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@huawei.com> > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- > 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@huawei.com/ > v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ > 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
On Tue, Sep 27, 2022 at 4:00 AM Wang Yufen <wangyufen@huawei.com> wrote: > > Add auto_attach optional to support one-step load-attach-pin_link. > > 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@huawei.com> > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- For next revision, please also attach cover letter describing the overall goal of the patch set (and that's where the version log between revisions is put as well). > 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@huawei.com/ > v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ > tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 79 insertions(+), 2 deletions(-) > [...]
在 2022/10/1 0:26, Quentin Monnet 写道: > Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com> >> 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@huawei.com> >> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >> --- >> 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@huawei.com/ >> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ >> 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: --- 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", - 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. Thanks, Wang
在 2022/10/1 4:55, Andrii Nakryiko 写道: > On Tue, Sep 27, 2022 at 4:00 AM Wang Yufen <wangyufen@huawei.com> wrote: >> Add auto_attach optional to support one-step load-attach-pin_link. >> >> 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@huawei.com> >> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >> --- > For next revision, please also attach cover letter describing the > overall goal of the patch set (and that's where the version log > between revisions is put as well). Thanks, will add a cover letter in v8. > > >> 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@huawei.com/ >> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ >> tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 79 insertions(+), 2 deletions(-) >> > [...] >
Sat Oct 08 2022 06:16:42 GMT+0100 ~ wangyufen <wangyufen@huawei.com> > > 在 2022/10/1 0:26, Quentin Monnet 写道: >> Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com> >>> 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@huawei.com> >>> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >>> --- >>> 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@huawei.com/ >>> v1: >>> https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ >>> 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). Looks good otherwise, thanks!
在 2022/10/10 16:40, Quentin Monnet 写道: > Sat Oct 08 2022 06:16:42 GMT+0100 ~ wangyufen <wangyufen@huawei.com> >> 在 2022/10/1 0:26, Quentin Monnet 写道: >>> Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com> >>>> 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@huawei.com> >>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >>>> --- >>>> 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@huawei.com/ >>>> v1: >>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ >>>> 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!
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; + } + + return 0; + +err_unpin_programs: + while ((prog = bpf_object__prev_program(obj, prog))) { + if (pathname_concat(path, bpf_program__name(prog), buf)) + continue; + + bpf_program__unpin(prog, buf); + } + + return err; +} + static int load_with_options(int argc, char **argv, bool first_prog_only) { enum bpf_prog_type common_prog_type = BPF_PROG_TYPE_UNSPEC; @@ -1464,6 +1530,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) struct bpf_program *prog = NULL, *pos; unsigned int old_map_fds = 0; const char *pinmaps = NULL; + bool auto_attach = false; struct bpf_object *obj; struct bpf_map *map; const char *pinfile; @@ -1583,6 +1650,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) goto err_free_reuse_maps; pinmaps = GET_ARG(); + } else if (is_prefix(*argv, "autoattach")) { + auto_attach = true; + NEXT_ARG(); } else { p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?", *argv); @@ -1692,14 +1762,20 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) goto err_close_obj; } - err = bpf_obj_pin(bpf_program__fd(prog), pinfile); + if (auto_attach) + err = auto_attach_program(prog, pinfile); + else + err = bpf_obj_pin(bpf_program__fd(prog), pinfile); if (err) { p_err("failed to pin program %s", bpf_program__section_name(prog)); goto err_close_obj; } } else { - err = bpf_object__pin_programs(obj, pinfile); + if (auto_attach) + err = auto_attach_programs(obj, pinfile); + else + err = bpf_object__pin_programs(obj, pinfile); if (err) { p_err("failed to pin all programs"); goto err_close_obj; @@ -2338,6 +2414,7 @@ static int do_help(int argc, char **argv) " [type TYPE] [dev NAME] \\\n" " [map { idx IDX | name NAME } MAP]\\\n" " [pinmaps MAP_DIR]\n" + " [autoattach]\n" " %1$s %2$s attach PROG ATTACH_TYPE [MAP]\n" " %1$s %2$s detach PROG ATTACH_TYPE [MAP]\n" " %1$s %2$s run PROG \\\n"