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@xxxxxxxxxx>
> ---
> v1 -> v2: switch to extend perf command instead add new one
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@xxxxxxxxxx/
> ---
> .../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
;;
