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

Quentin Monnet
2020-04-29T08:37:07Z

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