| Message ID | 20220824033837.458197-1-weiyongjun1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpftool: implement perf attach command | 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 | success | Single patches do not need cover letters |
| 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 14 of 14 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, 112 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-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 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 |
| 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-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| 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-13 | success | Logs for test_progs_no_alu32 on x86_64 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-10 | success | Logs for test_progs on x86_64 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-6 | success | Logs for test_maps on s390x 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-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
Hi Wei, Apologies for failing to answer to your previous email and for the delay on this one, I just found out GMail had classified them as spam :(. So as for your last message, yes: your understanding of my previous answer was correct. Thanks for the patch below! Some comments inline. On 24/08/2022 04:38, Wei Yongjun wrote: > This patch introduces a new bpftool command: perf attach, > which used to attaching/pinning tracepoints programs. > > bpftool perf attach PROG TP_NAME FILE > > It will attach bpf program PROG to tracepoint TP_NAME and > pin tracepoint program as FILE, FILE must be located in > bpffs mount. > > For example, > $ bpftool prog load mtd-mchp23k256.o /sys/fs/bpf/test_prog > > $ bpftool prog > 510: raw_tracepoint_writable name mtd_mchp23k256 tag 2e13281b1f781bf3 gpl > loaded_at 2022-08-24T02:50:06+0000 uid 0 > xlated 960B not jited memlock 4096B map_ids 439,437,440 > btf_id 740 > $ bpftool perf attach id 510 spi_transfer_writeable /sys/fs/bpf/test_perf How would you feel about adding more keywords to this syntax? $ bpftool perf attach id 510 attach_name <spi_...> path /sys/... A bit more parsing to do, but it's more flexible if we need or want more arguments in the future. We don't have to accept them in any random order, fixed order is fine (but having keywords allow us to support random order in the future). > > $ bpftool link show > 74: raw_tracepoint prog 510 > tp 'spi_transfer_writeable' > > The implementation a BPF based backend for mchp23k256 mtd mockup > device. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > v1 -> v2: switch to extend perf command instead add new one > v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ > --- > .../bpftool/Documentation/bpftool-perf.rst | 11 ++- > tools/bpf/bpftool/perf.c | 67 ++++++++++++++++++- > 2 files changed, 76 insertions(+), 2 deletions(-) > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst > index 5fea633a82f1..085c8dcfb9aa 100644 > --- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst > +++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst > @@ -19,12 +19,13 @@ SYNOPSIS > *OPTIONS* := { |COMMON_OPTIONS| } > > *COMMANDS* := > - { **show** | **list** | **help** } > + { **show** | **list** | **help** | **attach** } Missing (see bpftool-prog.rst): | *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* } > > PERF COMMANDS > ============= > > | **bpftool** **perf** { **show** | **list** } > +| **bpftool** **perf** **attach** *PROG* *TP_NAME* *FILE* Please rename TP_NAME into something else (here and below), maybe ATTACH_NAME or ATTACH_POINT, because we may support some other types in the future. > | **bpftool** **perf help** > > DESCRIPTION > @@ -39,6 +40,14 @@ DESCRIPTION > or a kernel virtual address. > The attachment point for u[ret]probe is the file name and the file offset. > > + **bpftool perf attach PROG TP_NAME FILE** > + Attach bpf program *PROG* to tracepoint *TP_NAME* and pin > + program *PROG* as *FILE*. Could you please add two notes on 1) how to detach the program (by deleting the link with rm, or does "bpftool link detach" work?), and 2) raw tracepoints (+ writable version) currently being the only supported program types for this command? > + > + Note: *FILE* must be located in *bpffs* mount. It must not > + contain a dot character ('.'), which is reserved for future > + extensions of *bpffs*. Note: It's not really future extensions anymore, but other parts of the docs say that, so let's keep this and we'll clean up everywhere at some point. > + > **bpftool perf help** > Print short help message. > > diff --git a/tools/bpf/bpftool/perf.c b/tools/bpf/bpftool/perf.c > index 226ec2c39052..9149ba960784 100644 > --- a/tools/bpf/bpftool/perf.c > +++ b/tools/bpf/bpftool/perf.c > @@ -233,8 +233,9 @@ static int do_show(int argc, char **argv) > static int do_help(int argc, char **argv) > { > fprintf(stderr, > - "Usage: %1$s %2$s { show | list }\n" > + "Usage: %1$s %2$s { show | list | attach }\n" > " %1$s %2$s help }\n" > + " %1$s %2$s attach PROG TP_NAME FILE\n" Should be: "Usage: %1$s %2$s { show | list }\n" " %1$s %2$s attach PROG ATTACH_NAME FILE\n" " %1$s %2$s help }\n" No need to list the subcommand twice, and let's keep "help" at the end. > "\n" > " " HELP_SPEC_OPTIONS " }\n" > "", > @@ -243,10 +244,74 @@ static int do_help(int argc, char **argv) > return 0; > } > > +static enum bpf_prog_type get_prog_type(int progfd) > +{ > + struct bpf_prog_info info = {}; > + __u32 len = sizeof(info); > + > + if (bpf_obj_get_info_by_fd(progfd, &info, &len)) > + return BPF_PROG_TYPE_UNSPEC; > + > + return info.type; > +} > + > +static int do_attach(int argc, char **argv) > +{ > + enum bpf_prog_type prog_type; > + char *tp_name, *path; > + int err, progfd, pfd; pfd -> pinfd? > + > + if (!REQ_ARGS(4)) > + return -EINVAL; > + > + progfd = prog_parse_fd(&argc, &argv); > + if (progfd < 0) > + return progfd; > + > + if (!REQ_ARGS(2)) { > + err = -EINVAL; > + goto out_close; > + } > + > + tp_name = GET_ARG(); > + path = GET_ARG(); > + > + prog_type = get_prog_type(progfd); > + switch (prog_type) { > + case BPF_PROG_TYPE_RAW_TRACEPOINT: > + case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: > + pfd = bpf_raw_tracepoint_open(tp_name, progfd); > + if (pfd < 0) { > + printf("failed to attach to raw tracepoint '%s'\n", > + tp_name); This should go to stderr. Please use: p_err("failed to attach to raw tracepoint '%s'", tp_name); (No line break needed.) > + err = pfd; > + goto out_close; > + } > + break; > + default: > + printf("invalid program type %s\n", > + libbpf_bpf_prog_type_str(prog_type)); p_err(). Also we could maybe mentioned that raw_tracepoint is the only type supported? > + err = -EINVAL; > + goto out_close; > + } > + > + err = do_pin_fd(pfd, path); > + if (err) { > + close(pfd); > + goto out_close; > + } > + > + return 0; No need for the last error check here. Initialise err to 0, then close pfd unconditionally and just go through out_close to close progfd then return err. Once the link is pinned, it's OK to close the file descriptors. They will close anyway when bpftool exits. > + > +out_close: "out_close" indeed, but the close(progfd) is missing :) > + return err; > +} Nitpick: I'd rather have do_attach() defined before do_help() in the file. That's probably just me being used to look for the usage strings at the bottom of most bpftool files. > + > static const struct cmd cmds[] = { > { "show", do_show }, > { "list", do_show }, > { "help", do_help }, > + { "attach", do_attach }, > { 0 } > }; > Please add the related bash completion in .../bpftool/bash-completion/bpftool. It should look like this: diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index dc1641e3670e..dd8d424e9a59 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -1080,10 +1080,37 @@ _bpftool() ;; perf) case $command in + attach) + local PROG_TYPE='id pinned tag name' + case $cword in + 3) + COMPREPLY=( $( compgen -W "$PROG_TYPE" -- "$cur" ) ) + return 0 + ;; + 4) + case $prev in + id) + _bpftool_get_prog_ids + ;; + name) + _bpftool_get_prog_names + ;; + esac + return 0 + ;; + 5) # Attach type + return 0 + ;; + 6) # Pinned link path + _filedir + return 0 + ;; + esac + ;; *) [[ $prev == $object ]] && \ COMPREPLY=( $( compgen -W 'help \ - show list' -- "$cur" ) ) + show list attach' -- "$cur" ) ) ;; esac ;;
On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: > > Hi Wei, > > Apologies for failing to answer to your previous email and for the delay > on this one, I just found out GMail had classified them as spam :(. > > So as for your last message, yes: your understanding of my previous > answer was correct. Thanks for the patch below! Some comments inline. > Do we really want to add such a specific command to bpftool that would attach BPF object files with programs of only RAW_TRACEPOINT and RAW_TRACEPOINT_WRITABLE type? I could understand if we added something that would be equivalent of BPF skeleton's auto-attach method. That would make sense in some contexts, especially for some quick testing and validation, to avoid writing (a rather simple) user-space loading code. But "perf attach" for raw_tp programs only? Seem way too limited and specific, just adding bloat to bpftool, IMO. > On 24/08/2022 04:38, Wei Yongjun wrote: > > This patch introduces a new bpftool command: perf attach, > > which used to attaching/pinning tracepoints programs. > > > > bpftool perf attach PROG TP_NAME FILE > > > > It will attach bpf program PROG to tracepoint TP_NAME and > > pin tracepoint program as FILE, FILE must be located in > > bpffs mount. > > [...]
Hi Andrii, On 25/08/2022 19:37, Andrii Nakryiko wrote: > On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> Hi Wei, >> >> Apologies for failing to answer to your previous email and for the delay >> on this one, I just found out GMail had classified them as spam :(. >> >> So as for your last message, yes: your understanding of my previous >> answer was correct. Thanks for the patch below! Some comments inline. >> > > Do we really want to add such a specific command to bpftool that would > attach BPF object files with programs of only RAW_TRACEPOINT and > RAW_TRACEPOINT_WRITABLE type? > > I could understand if we added something that would be equivalent of > BPF skeleton's auto-attach method. That would make sense in some > contexts, especially for some quick testing and validation, to avoid > writing (a rather simple) user-space loading code. Do you mean loading and attaching in a single step, or keeping the possibility to load first as in the current proposal? > > But "perf attach" for raw_tp programs only? Seem way too limited and > specific, just adding bloat to bpftool, IMO. We already support attaching some kinds of program types through "prog|cgroup|net attach". Here I thought we could add support for other types as a follow-up, but thinking again, you're probably right, it would be best if all the types were supported from the start. Wei, have you looked into how much work it would be to add support for tracepoints, k(ret)probes, u(ret)probes as well? The code should be mostly identical? Quentin
Hi Quentin, On 2022/8/26 18:45, Quentin Monnet wrote: > Hi Andrii, > > On 25/08/2022 19:37, Andrii Nakryiko wrote: >> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: >>> >>> Hi Wei, >>> >>> Apologies for failing to answer to your previous email and for the delay >>> on this one, I just found out GMail had classified them as spam :(. >>> >>> So as for your last message, yes: your understanding of my previous >>> answer was correct. Thanks for the patch below! Some comments inline. >>> >> >> Do we really want to add such a specific command to bpftool that would >> attach BPF object files with programs of only RAW_TRACEPOINT and >> RAW_TRACEPOINT_WRITABLE type? >> >> I could understand if we added something that would be equivalent of >> BPF skeleton's auto-attach method. That would make sense in some >> contexts, especially for some quick testing and validation, to avoid >> writing (a rather simple) user-space loading code. > > Do you mean loading and attaching in a single step, or keeping the > possibility to load first as in the current proposal? > >> >> But "perf attach" for raw_tp programs only? Seem way too limited and >> specific, just adding bloat to bpftool, IMO. > > We already support attaching some kinds of program types through > "prog|cgroup|net attach". Here I thought we could add support for other > types as a follow-up, but thinking again, you're probably right, it > would be best if all the types were supported from the start. Wei, have > you looked into how much work it would be to add support for > tracepoints, k(ret)probes, u(ret)probes as well? The code should be > mostly identical? Will post in next version with all other comments fixed after some testing for them. Regrads, Wei Yongjun
On Fri, Aug 26, 2022 at 3:45 AM Quentin Monnet <quentin@isovalent.com> wrote: > > Hi Andrii, > > On 25/08/2022 19:37, Andrii Nakryiko wrote: > > On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> Hi Wei, > >> > >> Apologies for failing to answer to your previous email and for the delay > >> on this one, I just found out GMail had classified them as spam :(. > >> > >> So as for your last message, yes: your understanding of my previous > >> answer was correct. Thanks for the patch below! Some comments inline. > >> > > > > Do we really want to add such a specific command to bpftool that would > > attach BPF object files with programs of only RAW_TRACEPOINT and > > RAW_TRACEPOINT_WRITABLE type? > > > > I could understand if we added something that would be equivalent of > > BPF skeleton's auto-attach method. That would make sense in some > > contexts, especially for some quick testing and validation, to avoid > > writing (a rather simple) user-space loading code. > > Do you mean loading and attaching in a single step, or keeping the > possibility to load first as in the current proposal? > > > > > But "perf attach" for raw_tp programs only? Seem way too limited and > > specific, just adding bloat to bpftool, IMO. > > We already support attaching some kinds of program types through > "prog|cgroup|net attach". Here I thought we could add support for other > types as a follow-up, but thinking again, you're probably right, it > would be best if all the types were supported from the start. Wei, have > you looked into how much work it would be to add support for > tracepoints, k(ret)probes, u(ret)probes as well? The code should be > mostly identical? Are you thinking to allow to attach individual BPF programs within BPF object, i.e., effectively bpftool as an interface to libbpf's bpf_program__attach()? Or you had more of BPF skeleton's auto-attach that attaches all auto-attachable BPF programs? > > Quentin >
On 2022/8/27 13:12, Andrii Nakryiko wrote: > On Fri, Aug 26, 2022 at 3:45 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> Hi Andrii, >> >> On 25/08/2022 19:37, Andrii Nakryiko wrote: >>> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: >>>> >>>> Hi Wei, >>>> >>>> Apologies for failing to answer to your previous email and for the delay >>>> on this one, I just found out GMail had classified them as spam :(. >>>> >>>> So as for your last message, yes: your understanding of my previous >>>> answer was correct. Thanks for the patch below! Some comments inline. >>>> >>> >>> Do we really want to add such a specific command to bpftool that would >>> attach BPF object files with programs of only RAW_TRACEPOINT and >>> RAW_TRACEPOINT_WRITABLE type? >>> >>> I could understand if we added something that would be equivalent of >>> BPF skeleton's auto-attach method. That would make sense in some >>> contexts, especially for some quick testing and validation, to avoid >>> writing (a rather simple) user-space loading code. >> >> Do you mean loading and attaching in a single step, or keeping the >> possibility to load first as in the current proposal? >> >>> >>> But "perf attach" for raw_tp programs only? Seem way too limited and >>> specific, just adding bloat to bpftool, IMO. >> >> We already support attaching some kinds of program types through >> "prog|cgroup|net attach". Here I thought we could add support for other >> types as a follow-up, but thinking again, you're probably right, it >> would be best if all the types were supported from the start. Wei, have >> you looked into how much work it would be to add support for >> tracepoints, k(ret)probes, u(ret)probes as well? The code should be >> mostly identical? > > Are you thinking to allow to attach individual BPF programs within BPF > object, i.e., effectively bpftool as an interface to libbpf's > bpf_program__attach()? My use case is attach/detach individual BPF programs. We used writeable raw tracepoint in mockup bus controller[1], individual BPF programs action as different mockup device chips, and the unittest can be running on the top of mockup device. So BPF programs should be attach/detach by the test framework. [1] https://www.spinics.net/lists/kernel/msg4490698.html > > Or you had more of BPF skeleton's auto-attach that attaches all > auto-attachable BPF programs? Regards, Wei Yongjun
On Sat, 27 Aug 2022 at 06:12, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Aug 26, 2022 at 3:45 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > Hi Andrii, > > > > On 25/08/2022 19:37, Andrii Nakryiko wrote: > > > On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: > > >> > > >> Hi Wei, > > >> > > >> Apologies for failing to answer to your previous email and for the delay > > >> on this one, I just found out GMail had classified them as spam :(. > > >> > > >> So as for your last message, yes: your understanding of my previous > > >> answer was correct. Thanks for the patch below! Some comments inline. > > >> > > > > > > Do we really want to add such a specific command to bpftool that would > > > attach BPF object files with programs of only RAW_TRACEPOINT and > > > RAW_TRACEPOINT_WRITABLE type? > > > > > > I could understand if we added something that would be equivalent of > > > BPF skeleton's auto-attach method. That would make sense in some > > > contexts, especially for some quick testing and validation, to avoid > > > writing (a rather simple) user-space loading code. > > > > Do you mean loading and attaching in a single step, or keeping the > > possibility to load first as in the current proposal? > > > > > > > > But "perf attach" for raw_tp programs only? Seem way too limited and > > > specific, just adding bloat to bpftool, IMO. > > > > We already support attaching some kinds of program types through > > "prog|cgroup|net attach". Here I thought we could add support for other > > types as a follow-up, but thinking again, you're probably right, it > > would be best if all the types were supported from the start. Wei, have > > you looked into how much work it would be to add support for > > tracepoints, k(ret)probes, u(ret)probes as well? The code should be > > mostly identical? > > Are you thinking to allow to attach individual BPF programs within BPF > object, i.e., effectively bpftool as an interface to libbpf's > bpf_program__attach()? > > Or you had more of BPF skeleton's auto-attach that attaches all > auto-attachable BPF programs? Here I was thinking the former, to have the support for tracing programs catch up with networking/cgroup programs that can be attached with bpftool already. Although for the future I'm also considering the second option, where we could pass an option to "bpftool prog load" to tell it to attach everything it can. Not sure yet, there are some similarities between the two (and my guess is you would find it too much?), but they're not exactly the same use case either.
Hi Quentin, On 2022/8/26 18:45, Quentin Monnet wrote: > Hi Andrii, > > On 25/08/2022 19:37, Andrii Nakryiko wrote: >> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet <quentin@isovalent.com> wrote: >>> >>> Hi Wei, >>> >>> Apologies for failing to answer to your previous email and for the delay >>> on this one, I just found out GMail had classified them as spam :(. >>> >>> So as for your last message, yes: your understanding of my previous >>> answer was correct. Thanks for the patch below! Some comments inline. >>> >> >> Do we really want to add such a specific command to bpftool that would >> attach BPF object files with programs of only RAW_TRACEPOINT and >> RAW_TRACEPOINT_WRITABLE type? >> >> I could understand if we added something that would be equivalent of >> BPF skeleton's auto-attach method. That would make sense in some >> contexts, especially for some quick testing and validation, to avoid >> writing (a rather simple) user-space loading code. > > Do you mean loading and attaching in a single step, or keeping the > possibility to load first as in the current proposal? > >> >> But "perf attach" for raw_tp programs only? Seem way too limited and >> specific, just adding bloat to bpftool, IMO. > > We already support attaching some kinds of program types through > "prog|cgroup|net attach". Here I thought we could add support for other > types as a follow-up, but thinking again, you're probably right, it > would be best if all the types were supported from the start. Wei, have > you looked into how much work it would be to add support for > tracepoints, k(ret)probes, u(ret)probes as well? The code should be > mostly identical? > When I try to add others support, I found that we need to dup many code with libbpf has already done, since we lost the section name info. I have tried to add auto-attach, it seems more easier then perf attach command. What's about your opinion? Maybe we only need a little of changes like this: $ bpftool prog load test.o /sys/fs/bpf/test auto-attach diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index c81362a001ba..87fab89eaa07 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -1464,6 +1464,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; @@ -1472,7 +1473,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) const char *file; int idx, err; - if (!REQ_ARGS(2)) return -1; file = GET_ARG(); @@ -1583,6 +1583,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, "auto-attach")) { + auto_attach = true; + NEXT_ARG(); } else { p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?", *argv); @@ -1692,13 +1695,17 @@ 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); + bpf_program__set_autoattach(prog, auto_attach); + err = bpf_program__pin(prog, pinfile); if (err) { p_err("failed to pin program %s", bpf_program__section_name(prog)); goto err_close_obj; } } else { + bpf_object__for_each_program(prog, obj) { + bpf_program__set_autoattach(prog, auto_attach); + } err = bpf_object__pin_programs(obj, pinfile); if (err) { p_err("failed to pin all programs"); diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 3ad139285fad..915ec0a97583 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -7773,15 +7773,32 @@ int bpf_program__pin(struct bpf_program *prog, const char *path) if (err) return libbpf_err(err); - if (bpf_obj_pin(prog->fd, path)) { - err = -errno; - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, cp); - return libbpf_err(err); + if (prog->autoattach) { + struct bpf_link *link; + + link = bpf_program__attach(prog); + err = libbpf_get_error(link); + if (err) + goto err_out; + + err = bpf_link__pin(link, path); + if (err) { + bpf_link__destroy(link); + goto err_out; + } + } else { + if (bpf_obj_pin(prog->fd, path)) { + err = -errno; + goto err_out; + } } pr_debug("prog '%s': pinned at '%s'\n", prog->name, path); return 0; +err_out: + cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); + pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, cp); + return libbpf_err(err); } int bpf_program__unpin(struct bpf_program *prog, const char *path)
On 02/09/2022 11:23, weiyongjun (A) wrote: > Hi Quentin, > > On 2022/8/26 18:45, Quentin Monnet wrote: >> Hi Andrii, >> >> On 25/08/2022 19:37, Andrii Nakryiko wrote: >>> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet >>> <quentin@isovalent.com> wrote: >>>> >>>> Hi Wei, >>>> >>>> Apologies for failing to answer to your previous email and for the >>>> delay >>>> on this one, I just found out GMail had classified them as spam :(. >>>> >>>> So as for your last message, yes: your understanding of my previous >>>> answer was correct. Thanks for the patch below! Some comments inline. >>>> >>> >>> Do we really want to add such a specific command to bpftool that would >>> attach BPF object files with programs of only RAW_TRACEPOINT and >>> RAW_TRACEPOINT_WRITABLE type? >>> >>> I could understand if we added something that would be equivalent of >>> BPF skeleton's auto-attach method. That would make sense in some >>> contexts, especially for some quick testing and validation, to avoid >>> writing (a rather simple) user-space loading code. >> >> Do you mean loading and attaching in a single step, or keeping the >> possibility to load first as in the current proposal? >> >>> >>> But "perf attach" for raw_tp programs only? Seem way too limited and >>> specific, just adding bloat to bpftool, IMO. >> >> We already support attaching some kinds of program types through >> "prog|cgroup|net attach". Here I thought we could add support for other >> types as a follow-up, but thinking again, you're probably right, it >> would be best if all the types were supported from the start. Wei, have >> you looked into how much work it would be to add support for >> tracepoints, k(ret)probes, u(ret)probes as well? The code should be >> mostly identical? >> > > > When I try to add others support, I found that we need to dup many code > with libbpf has already done, since we lost the section name info. What amount of code does this represent? Do you have a version of the patch accessible somewhere? I trust you, I'm just curious about the steps we're missing without having processed the section name info, but I can go and look at the details myself otherwise. > I have tried to add auto-attach, it seems more easier then perf > attach command. > > What's about your opinion? Yes I'm good with that approach, too. > Maybe we only need a little of changes like this: > > $ bpftool prog load test.o /sys/fs/bpf/test auto-attach "auto_attach", other keywords use underscores rather than dashes. > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index c81362a001ba..87fab89eaa07 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -1464,6 +1464,7 @@ static int load_with_options(int argc, char > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 3ad139285fad..915ec0a97583 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -7773,15 +7773,32 @@ int bpf_program__pin(struct bpf_program *prog, > const char *path) > if (err) > return libbpf_err(err); > > - if (bpf_obj_pin(prog->fd, path)) { > - err = -errno; > - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); > - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, > path, cp); > - return libbpf_err(err); > + if (prog->autoattach) { > + struct bpf_link *link; > + > + link = bpf_program__attach(prog); > + err = libbpf_get_error(link); > + if (err) > + goto err_out; > + > + err = bpf_link__pin(link, path); > + if (err) { > + bpf_link__destroy(link); > + goto err_out; > + } > + } else { > + if (bpf_obj_pin(prog->fd, path)) { > + err = -errno; > + goto err_out; > + } > } > > pr_debug("prog '%s': pinned at '%s'\n", prog->name, path); > return 0; > +err_out: > + cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); > + pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, > cp); > + return libbpf_err(err); > } > > int bpf_program__unpin(struct bpf_program *prog, const char *path) I don't think it is correct to modify libbpf's bpf_program__pin() though, because it shouldn't be its role to attach and also because I think it might lead to a second attempt to attach if the user tries to pin in addition to running the auto-attach from a skeleton. Let's just call bpf_program__attach() from bpftool instead?
On Tue, Sep 6, 2022 at 2:17 AM Quentin Monnet <quentin@isovalent.com> wrote: > > On 02/09/2022 11:23, weiyongjun (A) wrote: > > Hi Quentin, > > > > On 2022/8/26 18:45, Quentin Monnet wrote: > >> Hi Andrii, > >> > >> On 25/08/2022 19:37, Andrii Nakryiko wrote: > >>> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet > >>> <quentin@isovalent.com> wrote: > >>>> > >>>> Hi Wei, > >>>> > >>>> Apologies for failing to answer to your previous email and for the > >>>> delay > >>>> on this one, I just found out GMail had classified them as spam :(. > >>>> > >>>> So as for your last message, yes: your understanding of my previous > >>>> answer was correct. Thanks for the patch below! Some comments inline. > >>>> > >>> > >>> Do we really want to add such a specific command to bpftool that would > >>> attach BPF object files with programs of only RAW_TRACEPOINT and > >>> RAW_TRACEPOINT_WRITABLE type? > >>> > >>> I could understand if we added something that would be equivalent of > >>> BPF skeleton's auto-attach method. That would make sense in some > >>> contexts, especially for some quick testing and validation, to avoid > >>> writing (a rather simple) user-space loading code. > >> > >> Do you mean loading and attaching in a single step, or keeping the > >> possibility to load first as in the current proposal? > >> > >>> > >>> But "perf attach" for raw_tp programs only? Seem way too limited and > >>> specific, just adding bloat to bpftool, IMO. > >> > >> We already support attaching some kinds of program types through > >> "prog|cgroup|net attach". Here I thought we could add support for other > >> types as a follow-up, but thinking again, you're probably right, it > >> would be best if all the types were supported from the start. Wei, have > >> you looked into how much work it would be to add support for > >> tracepoints, k(ret)probes, u(ret)probes as well? The code should be > >> mostly identical? > >> > > > > > > When I try to add others support, I found that we need to dup many code > > with libbpf has already done, since we lost the section name info. I don't think bpftool should be parsing SEC() definitions. As Quentin suggested, just bpf_program__attach() should be enough. > > What amount of code does this represent? Do you have a version of the > patch accessible somewhere? I trust you, I'm just curious about the > steps we're missing without having processed the section name info, but > I can go and look at the details myself otherwise. > > > I have tried to add auto-attach, it seems more easier then perf > > attach command. > > > > What's about your opinion? > > Yes I'm good with that approach, too. > > > Maybe we only need a little of changes like this: > > > > $ bpftool prog load test.o /sys/fs/bpf/test auto-attach > > "auto_attach", other keywords use underscores rather than dashes. > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > index c81362a001ba..87fab89eaa07 100644 > > --- a/tools/bpf/bpftool/prog.c > > +++ b/tools/bpf/bpftool/prog.c > > @@ -1464,6 +1464,7 @@ static int load_with_options(int argc, char > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 3ad139285fad..915ec0a97583 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -7773,15 +7773,32 @@ int bpf_program__pin(struct bpf_program *prog, > > const char *path) > > if (err) > > return libbpf_err(err); > > > > - if (bpf_obj_pin(prog->fd, path)) { > > - err = -errno; > > - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); > > - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, > > path, cp); > > - return libbpf_err(err); > > + if (prog->autoattach) { > > + struct bpf_link *link; > > + > > + link = bpf_program__attach(prog); > > + err = libbpf_get_error(link); > > + if (err) > > + goto err_out; > > + > > + err = bpf_link__pin(link, path); > > + if (err) { > > + bpf_link__destroy(link); > > + goto err_out; > > + } > > + } else { > > + if (bpf_obj_pin(prog->fd, path)) { > > + err = -errno; > > + goto err_out; > > + } > > } > > > > pr_debug("prog '%s': pinned at '%s'\n", prog->name, path); > > return 0; > > +err_out: > > + cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); > > + pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, > > cp); > > + return libbpf_err(err); > > } > > > > int bpf_program__unpin(struct bpf_program *prog, const char *path) > > I don't think it is correct to modify libbpf's bpf_program__pin() > though, because it shouldn't be its role to attach and also because I > think it might lead to a second attempt to attach if the user tries to > pin in addition to running the auto-attach from a skeleton. Let's just > call bpf_program__attach() from bpftool instead? +1, libbpf shouldn't be modified for this feature
diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst index 5fea633a82f1..085c8dcfb9aa 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst @@ -19,12 +19,13 @@ SYNOPSIS *OPTIONS* := { |COMMON_OPTIONS| } *COMMANDS* := - { **show** | **list** | **help** } + { **show** | **list** | **help** | **attach** } PERF COMMANDS ============= | **bpftool** **perf** { **show** | **list** } +| **bpftool** **perf** **attach** *PROG* *TP_NAME* *FILE* | **bpftool** **perf help** DESCRIPTION @@ -39,6 +40,14 @@ DESCRIPTION or a kernel virtual address. The attachment point for u[ret]probe is the file name and the file offset. + **bpftool perf attach PROG TP_NAME FILE** + Attach bpf program *PROG* to tracepoint *TP_NAME* and pin + program *PROG* as *FILE*. + + Note: *FILE* must be located in *bpffs* mount. It must not + contain a dot character ('.'), which is reserved for future + extensions of *bpffs*. + **bpftool perf help** Print short help message. diff --git a/tools/bpf/bpftool/perf.c b/tools/bpf/bpftool/perf.c index 226ec2c39052..9149ba960784 100644 --- a/tools/bpf/bpftool/perf.c +++ b/tools/bpf/bpftool/perf.c @@ -233,8 +233,9 @@ static int do_show(int argc, char **argv) static int do_help(int argc, char **argv) { fprintf(stderr, - "Usage: %1$s %2$s { show | list }\n" + "Usage: %1$s %2$s { show | list | attach }\n" " %1$s %2$s help }\n" + " %1$s %2$s attach PROG TP_NAME FILE\n" "\n" " " HELP_SPEC_OPTIONS " }\n" "", @@ -243,10 +244,74 @@ static int do_help(int argc, char **argv) return 0; } +static enum bpf_prog_type get_prog_type(int progfd) +{ + struct bpf_prog_info info = {}; + __u32 len = sizeof(info); + + if (bpf_obj_get_info_by_fd(progfd, &info, &len)) + return BPF_PROG_TYPE_UNSPEC; + + return info.type; +} + +static int do_attach(int argc, char **argv) +{ + enum bpf_prog_type prog_type; + char *tp_name, *path; + int err, progfd, pfd; + + if (!REQ_ARGS(4)) + return -EINVAL; + + progfd = prog_parse_fd(&argc, &argv); + if (progfd < 0) + return progfd; + + if (!REQ_ARGS(2)) { + err = -EINVAL; + goto out_close; + } + + tp_name = GET_ARG(); + path = GET_ARG(); + + prog_type = get_prog_type(progfd); + switch (prog_type) { + case BPF_PROG_TYPE_RAW_TRACEPOINT: + case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: + pfd = bpf_raw_tracepoint_open(tp_name, progfd); + if (pfd < 0) { + printf("failed to attach to raw tracepoint '%s'\n", + tp_name); + err = pfd; + goto out_close; + } + break; + default: + printf("invalid program type %s\n", + libbpf_bpf_prog_type_str(prog_type)); + err = -EINVAL; + goto out_close; + } + + err = do_pin_fd(pfd, path); + if (err) { + close(pfd); + goto out_close; + } + + return 0; + +out_close: + return err; +} + static const struct cmd cmds[] = { { "show", do_show }, { "list", do_show }, { "help", do_help }, + { "attach", do_attach }, { 0 } };
This patch introduces a new bpftool command: perf attach, which used to attaching/pinning tracepoints programs. bpftool perf attach PROG TP_NAME FILE It will attach bpf program PROG to tracepoint TP_NAME and pin tracepoint program as FILE, FILE must be located in bpffs mount. For example, $ bpftool prog load mtd-mchp23k256.o /sys/fs/bpf/test_prog $ bpftool prog 510: raw_tracepoint_writable name mtd_mchp23k256 tag 2e13281b1f781bf3 gpl loaded_at 2022-08-24T02:50:06+0000 uid 0 xlated 960B not jited memlock 4096B map_ids 439,437,440 btf_id 740 $ bpftool perf attach id 510 spi_transfer_writeable /sys/fs/bpf/test_perf $ bpftool link show 74: raw_tracepoint prog 510 tp 'spi_transfer_writeable' The implementation a BPF based backend for mchp23k256 mtd mockup device. Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- v1 -> v2: switch to extend perf command instead add new one v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/ --- .../bpftool/Documentation/bpftool-perf.rst | 11 ++- tools/bpf/bpftool/perf.c | 67 ++++++++++++++++++- 2 files changed, 76 insertions(+), 2 deletions(-)