diff mbox series

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

Message ID 20200427201253.2996156-1-yhs@fb.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series bpf: implement bpf iterator for kernel data | expand

Commit Message

Yonghong Song April 27, 2020, 8:12 p.m. UTC
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

Comments

Quentin Monnet April 28, 2020, 9:27 a.m. UTC | #1
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
Yonghong Song April 28, 2020, 5:35 p.m. UTC | #2
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
>
Quentin Monnet April 29, 2020, 8:37 a.m. UTC | #3
2020-04-28 10:35 UTC-0700 ~ Yonghong Song <yhs@fb.com>
> 
> 
> On 4/28/20 2:27 AM, Quentin Monnet wrote:

[...]

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

Right, passing bpf_link__fd() to do_pin_any() wouldn't work, it does not
take the arguments expected by the "get_fd()" callback. My bad. So yeah,
just do_pin_fd() in that case :)

[...]

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

Ok, so with the useless bpffs pinning step and the perspectives for
other subcommands in the future, I agree it makes sense to have "iter"
as a new command. And as you say, handling of the link may grow so it's
probably not a bad thing to have it aside from the "prog" command.
Thanks for the clarification (maybe add some of it to the commit log
indeed?).

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

My point was more about loading programs that reuse pre-existing, as in
"bpftool prog load foo /sys/fs/bpf/foo map name foomap id 1337". It
seems likely that similar syntax will be needed for loading/pinning
iterators as well eventually, but I suppose we can try to refactor the
code from prog.c to reuse it when the time comes.

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

Right, I'm just not convinced that all users are aware of that :) But
fair enough.

> 
> BTW, Thanks for reviewing and catching my mistakes!
> 

Thanks for your reply and clarification, that's appreciated too!
Quentin
diff mbox series

Patch

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
+===================
+
+|	**bpftool** **iter pin** *OBJ* *PATH*
+|	**bpftool** **struct_ops help**
+|
+|	*OBJ* := /a/file/of/bpf_iter_target.o
+
+
+DESCRIPTION
+===========
+	**bpftool iter pin** *OBJ* *PATH*
+		  Create a bpf iterator from *OBJ*, and pin it to
+		  *PATH*. The *PATH* should be located in *bpffs* mount.
+
+	**bpftool struct_ops help**
+		  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" ) )
+                    ;;
+            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;
+
+	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);
+	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);
+}
diff --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);