Re: [PATCH bpf-next v1 16/19] tools/bpftool: add bpf_iter support for bptool

[Quentin Monnet <quentin@isovalent.com>]

2020-04-27 13:12 UTC-0700 ~ Yonghong Song <yhs@fb.com>
> Currently, only one command is supported
>   bpftool iter pin <bpf_prog.o> <path>
> 
> It will pin the trace/iter bpf program in
> the object file <bpf_prog.o> to the <path>
> where <path> should be on a bpffs mount.
> 
> For example,
>   $ bpftool iter pin ./bpf_iter_ipv6_route.o \
>     /sys/fs/bpf/my_route
> User can then do a `cat` to print out the results:
>   $ cat /sys/fs/bpf/my_route
>     fe800000000000000000000000000000 40 00000000000000000000000000000000 ...
>     00000000000000000000000000000000 00 00000000000000000000000000000000 ...
>     00000000000000000000000000000001 80 00000000000000000000000000000000 ...
>     fe800000000000008c0162fffebdfd57 80 00000000000000000000000000000000 ...
>     ff000000000000000000000000000000 08 00000000000000000000000000000000 ...
>     00000000000000000000000000000000 00 00000000000000000000000000000000 ...
> 
> The implementation for ipv6_route iterator is in one of subsequent
> patches.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  .../bpftool/Documentation/bpftool-iter.rst    | 71 ++++++++++++++++
>  tools/bpf/bpftool/bash-completion/bpftool     | 13 +++
>  tools/bpf/bpftool/iter.c                      | 84 +++++++++++++++++++
>  tools/bpf/bpftool/main.c                      |  3 +-
>  tools/bpf/bpftool/main.h                      |  1 +
>  5 files changed, 171 insertions(+), 1 deletion(-)
>  create mode 100644 tools/bpf/bpftool/Documentation/bpftool-iter.rst
>  create mode 100644 tools/bpf/bpftool/iter.c
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
> new file mode 100644
> index 000000000000..1997a6bac4a0
> --- /dev/null
> +++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
> @@ -0,0 +1,71 @@
> +============
> +bpftool-iter
> +============
> +-------------------------------------------------------------------------------
> +tool to create BPF iterators
> +-------------------------------------------------------------------------------
> +
> +:Manual section: 8
> +
> +SYNOPSIS
> +========
> +
> +	**bpftool** [*OPTIONS*] **iter** *COMMAND*
> +
> +	*COMMANDS* := { **pin** | **help** }
> +
> +STRUCT_OPS COMMANDS

s/STRUCT_OPS/ITER/

> +===================
> +
> +|	**bpftool** **iter pin** *OBJ* *PATH*
> +|	**bpftool** **struct_ops help**

s/struct_ops/iter/

> +|
> +|	*OBJ* := /a/file/of/bpf_iter_target.o
> +
> +
> +DESCRIPTION
> +===========
> +	**bpftool iter pin** *OBJ* *PATH*

Would be great to have a small blurb on what BPF iterators are and what
they can do. I'm afraid users reading this man page will have no idea
whatsoever.

> +		  Create a bpf iterator from *OBJ*, and pin it to
> +		  *PATH*. The *PATH* should be located in *bpffs* mount.

Can you keep the note that other pages have about the dot character
being forbidden in *PATH* basename, please?

> +
> +	**bpftool struct_ops help**

s/struct_ops/iter/

> +		  Print short help message.
> +
> +OPTIONS
> +=======
> +	-h, --help
> +		  Print short generic help message (similar to **bpftool help**).
> +
> +	-V, --version
> +		  Print version number (similar to **bpftool version**).
> +
> +	-d, --debug
> +		  Print all logs available, even debug-level information. This
> +		  includes logs from libbpf as well as from the verifier, when
> +		  attempting to load programs.> +
> +EXAMPLES
> +========
> +**# bpftool iter pin bpf_iter_netlink.o /sys/fs/bpf/my_netlink**
> +
> +::
> +
> +   Create a file-based bpf iterator from bpf_iter_netlink.o and pin it
> +   to /sys/fs/bpf/my_netlink
> +
> +
> +SEE ALSO
> +========
> +	**bpf**\ (2),
> +	**bpf-helpers**\ (7),
> +	**bpftool**\ (8),
> +	**bpftool-prog**\ (8),
> +	**bpftool-map**\ (8),
> +	**bpftool-cgroup**\ (8),
> +	**bpftool-feature**\ (8),
> +	**bpftool-net**\ (8),
> +	**bpftool-perf**\ (8),
> +	**bpftool-btf**\ (8)
> +	**bpftool-gen**\ (8)
> +	**bpftool-struct_ops**\ (8)
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 45ee99b159e2..17a81695da0f 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -604,6 +604,19 @@ _bpftool()
>                      ;;
>              esac
>              ;;
> +        iter)
> +            case $command in
> +                pin)
> +                    _filedir
> +                    return 0
> +                    ;;
> +                *)
> +                    [[ $prev == $object ]] && \
> +                        COMPREPLY=( $( compgen -W 'help' \
> +                            -- "$cur" ) )

You should probably offer "pin" here in addition to "help".

> +                    ;;
> +            esac
> +            ;;
>          map)
>              local MAP_TYPE='id pinned name'
>              case $command in
> diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
> new file mode 100644
> index 000000000000..db9fae6be716
> --- /dev/null
> +++ b/tools/bpf/bpftool/iter.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +// Copyright (C) 2020 Facebook
> +
> +#define _GNU_SOURCE
> +#include <linux/err.h>
> +#include <bpf/libbpf.h>
> +
> +#include "main.h"
> +
> +static int do_pin(int argc, char **argv)
> +{
> +	const char *objfile, *path;
> +	struct bpf_program *prog;
> +	struct bpf_object *obj;
> +	struct bpf_link *link;
> +	int err;

Nit: initialise err t0 -1 do you don't have to set it three times below?

> +
> +	if (!REQ_ARGS(2))
> +		usage();
> +
> +	objfile = GET_ARG();
> +	path = GET_ARG();
> +
> +	obj = bpf_object__open(objfile);
> +	if (IS_ERR_OR_NULL(obj)) {
> +		p_err("can't open objfile %s", objfile);
> +		return -1;
> +	}
> +
> +	err = bpf_object__load(obj);
> +	if (err < 0) {
> +		err = -1;
> +		p_err("can't load objfile %s", objfile);
> +		goto close_obj;
> +	}
> +
> +	prog = bpf_program__next(NULL, obj);
> +	link = bpf_program__attach_iter(prog, NULL);
> +	if (IS_ERR(link)) {
> +		err = -1;
> +		p_err("attach_iter failed for program %s",
> +		      bpf_program__name(prog));
> +		goto close_obj;
> +	}
> +
> +	err = bpf_link__pin(link, path);

Try to mount bpffs before that if "-n" is not passed? You could even
call do_pin_any() from common.c by passing bpf_link__fd().

> +	if (err) {
> +		err = -1;
> +		p_err("pin_iter failed for program %s to path %s",
> +		      bpf_program__name(prog), path);
> +		goto close_link;
> +	}
> +
> +	err = 0;
> +
> +close_link:
> +	bpf_link__disconnect(link);
> +	bpf_link__destroy(link);
> +close_obj:
> +	bpf_object__close(obj);
> +	return err;
> +}
> +
> +static int do_help(int argc, char **argv)
> +{
> +	fprintf(stderr,
> +		"Usage: %s %s pin OBJ PATH\n"
> +		"       %s %s help\n"
> +		"\n",
> +		bin_name, argv[-2], bin_name, argv[-2]);
> +
> +	return 0;
> +}
> +
> +static const struct cmd cmds[] = {
> +	{ "help",	do_help },
> +	{ "pin",	do_pin },
> +	{ 0 }
> +};
> +
> +int do_iter(int argc, char **argv)
> +{
> +	return cmd_select(cmds, argc, argv, do_help);
> +}
> dif	"",
>  		bin_name, bin_name, bin_name);
> @@ -222,6 +222,7 @@ static const struct cmd cmds[] = {
>  	{ "btf",	do_btf },
>  	{ "gen",	do_gen },
>  	{ "struct_ops",	do_struct_ops },
> +	{ "iter",	do_iter },
>  	{ "version",	do_version },
>  	{ 0 }
>  };
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 86f14ce26fd7..2b5d4a616b48 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -162,6 +162,7 @@ int do_feature(int argc, char **argv);
>  int do_btf(int argc, char **argv);
>  int do_gen(int argc, char **argv);
>  int do_struct_ops(int argc, char **argv);
> +int do_iter(int argc, char **argv);
>  
>  int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>  int prog_parse_fd(int *argc, char ***argv);
> f --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 466c269eabdd..6805b77789cb 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -58,7 +58,7 @@ static int do_help(int argc, char **argv)
>  		"       %s batch file FILE\n"
>  		"       %s version\n"
>  		"\n"
> -		"       OBJECT := { prog | map | cgroup | perf | net | feature | btf | gen | struct_ops }\n"
> +		"       OBJECT := { prog | map | cgroup | perf | net | feature | btf | gen | struct_ops | iter }\n"
>  		"       " HELP_SPEC_OPTIONS "\n"
>  		"",
>  		bin_name, bin_name, bin_name);
> @@ -222,6 +222,7 @@ static const struct cmd cmds[] = {
>  	{ "btf",	do_btf },
>  	{ "gen",	do_gen },
>  	{ "struct_ops",	do_struct_ops },
> +	{ "iter",	do_iter },
>  	{ "version",	do_version },
>  	{ 0 }
>  };
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 86f14ce26fd7..2b5d4a616b48 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -162,6 +162,7 @@ int do_feature(int argc, char **argv);
>  int do_btf(int argc, char **argv);
>  int do_gen(int argc, char **argv);
>  int do_struct_ops(int argc, char **argv);
> +int do_iter(int argc, char **argv);
>  
>  int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>  int prog_parse_fd(int *argc, char ***argv);
> 

Have you considered simply adapting the more traditional workflow
"bpftool prog load && bpftool prog attach" so that it supports iterators
instead of adding a new command? It would:

- Avoid adding yet another bpftool command with a single subcommand

- Enable to reuse the code from prog load, in particular for map reuse
(I'm not sure how relevant maps are for iterators, but I wouldn't be
surprised if someone finds a use case at some point?)

- Avoid users naively trying to run "bpftool prog load && bpftool prog
attach <prog> iter" and not understanding why it fails

Quentin