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