Date:   Wed, 29 Apr 2020 09:37:03 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        bpf@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
        netdev@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, kernel-team@...com
Subject: Re: [PATCH bpf-next v1 16/19] tools/bpftool: add bpf_iter support for
 bptool

2020-04-28 10:35 UTC-0700 ~ Yonghong Song <yhs@...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

Powered by blists - more mailing lists