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

[Yonghong Song <yhs@fb.com>]

On 4/28/20 2:27 AM, Quentin Monnet wrote:
> 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/

Oops. copy-paste error. Will fix.

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

Will fix.

> 
>> +|
>> +|	*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.

Will add.

> 
>> +		  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?

Will add.

> 
>> +
>> +	**bpftool struct_ops help**
> 
> s/struct_ops/iter/

Will fix.

> 
>> +		  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".

Will add.

> 
>> +                    ;;
>> +            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?

Double checked cmd_select() handling the return value:
   0 : success
   non-0 : failure

Looking like I can remove two `err = -1` 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().

You probably means do_pin_fd()? That is a good suggestion, will use it
in the next revision.

> 
>> +	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:

This is a good question, I should have clarified better in the commit
message.
   - prog load && prog attach won't work.
     the create_iter is a three stage process:
       1. prog load
       2. create and attach to a link
       3. pin link
     In the current implementation, the link merely just has the program.
     But in the future, the link will have other parameters like map_id,
     tgid/gid, or cgroup_id, or others.

     We could say to do:
       1. bpftool prog load <pin_path>
       2. bpftool iter pin prog file
          <maybe more parameters in the future>

     But this requires to pin the program itself in the bpffs, which
     mostly unneeded for file iterator creator.

     So this command `bpftool iter pin ...` is created for ease of use.

> 
> - Avoid adding yet another bpftool command with a single subcommand

So far, yes, in the future we may have more. In my RFC patcch, I have
`bpftool iter show ...` for introspection, this is to show all 
registered targets and all file iterators prog_id's.

This patch does not have it and I left it for the future work.
I am considering to use bpf iterator to do introspection here...

> 
> - 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?)

Yes, we do plan to have map element iterators. We can also have
bpf_prog or other iterators. Yes, map element iterator use 
implementation should be `bpftool map` code base since it is
a use of bpf_iter infrastructure.

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

`bpftool prog attach <prog> [map_id]` mostly used to attach a program to
a map, right? In this case, it won't apply, right?

BTW, Thanks for reviewing and catching my mistakes!

> 
> Quentin
>