Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Thu, 8 Nov 2018 13:29:40 -0800, Stanislav Fomichev wrote: > On 11/08, Jakub Kicinski wrote: > > On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote: > > > This commit adds support for loading/attaching/detaching flow > > > dissector program. The structure of the flow dissector program is > > > assumed to be the same as in the selftests: > > > > > > * flow_dissector section with the main entry point > > > * a bunch of tail call progs > > > * a jmp_table map that is populated with the tail call progs > > > > Could you split the loadall changes and the flow_dissector changes into > > two separate patches? > Sure, will do, but let's first agree on the semantical differences of > load vs loadall. > > So far *load* actually loads _all_ progs (via bpf_object__load), but pins > only the first program. Is that what we want? I wonder whether the > assumption there was that there is only single program in the object. > Should we load only the first program in *load*? > > If we add *loadall*, then the difference would be: > *load*: > * loads all maps and only the first program, pins only the first > program > *loadall*: > * loads all maps and all programs, pins everything (maps and programs) > > Is this the expected behavior? Loading all programs and maps for "load" is just a libbpf limitation we can remove at some point.
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Thu, 8 Nov 2018 13:25:39 -0800, Stanislav Fomichev wrote:
> > > > + goto err_close_obj;
> > > > + }
> > > > +
> > > > const char *sec_name = bpf_program__title(prog, false);
> > > >
> > > > err = libbpf_prog_type_by_name(sec_name,
> > > > _type,
> > > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > > > goto err_close_obj;
> > > > }
> > > > }
> > > > - bpf_program__set_type(prog, attr.prog_type);
> > > > - bpf_program__set_expected_attach_type(prog,
> > > > expected_attach_type);
> > > > +
> > > > + bpf_object__for_each_program(pos, obj) {
> > > > + bpf_program__set_ifindex(pos, ifindex);
> > > > + bpf_program__set_type(pos, attr.prog_type);
> > > > + bpf_program__set_expected_attach_type(pos,
> > > > +
> > > > expected_attach_type);
> > > > + }
> > >
> > > I still believe you can have programs of different types here, and be
> > > able to load them. I tried it and managed to have it working fine. If no
> > > type is provided from command line we can retrieve types for each
> > > program from its section name. If a type is provided on the command
> > > line, we can do the same, but I am not sure we should do it, or impose
> > > that type for all programs instead.
> >
> > attr->prog_type is one per object, though. How do we set that one?
> Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
> version in the bpf prog?
>
> It will probably work quite nicely for both of our options:
>
> * type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
> version (over cautious?)
> * type specified (and applied to all progs): using bpf_prog_type__needs_kver
> to require/not requqire kernel version
Right, but they you can't infer it from the program name, since there's
multiple.
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Thu, 8 Nov 2018 13:20:12 -0800, Stanislav Fomichev wrote:
> On 11/08, Jakub Kicinski wrote:
> > On Thu, 8 Nov 2018 18:21:24 +, Quentin Monnet wrote:
> > > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > > >>> contain a dot character ('.'), which is reserved for
> > > >>> future
> > > >>> extensions of *bpffs*.
> > > >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> > > >>> {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > > >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type**
> > > >>> *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev**
> > > >>> *NAME*]
> > > >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> > > >>> + **bpftool prog load** will pin only the first bpf
> > > >>> program
> > > >>> + from the *OBJ*, **bpftool prog loadall** will pin all
> > > >>> maps
> > > >>> + and programs from the *OBJ*.
> > > >>
> > > >> This could be improved regarding maps: with "bpftool prog load" I
> > > >> think we
> > > >> also load and pin all maps, but your description implies this is only
> > > >> the
> > > >> case with "loadall"
> > > > I don't think we pin any maps with `bpftool prog load`, we certainly
> > > > load
> > > > them, but we don't pin any afaict. Can you point me to the code where we
> > > > pin the maps?
> > > >
> > >
> > > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > > sorry about that.
> >
> > Right, but I don't see much reason why prog loadall should pin maps.
> It does seem convenient to have an option to pin everything, so we
> don't require anything else to find the ids of the maps.
> If we are pinning all progs, might as well pin the maps, why not? See
> my example in the commit message, for example, where I just hard code
> the expected map name. Convenient :-)
Sure. I can see how its convenient to your use case. A lot of people
will be very used to finding out the maps because if program is loaded
with iproute2 tools neither programs nor maps get pinned.
Please add a "pinmaps MAP_DIR" optional parameter as a separate patch.
> > The reason to pin program(s) is to hold some reference and to be able
> > to find them. Since we have the programs pinned we should be able to
> > find their maps with relative ease.
> >
> > $ bpftool prog show pinned /sys/fs/bpf/prog
> > 7: cgroup_skb tag 2a142ef67aaad174 gpl
> > loaded_at 2018-11-08T11:02:25-0800 uid 0
> > xlated 296B jited 229B memlock 4096B map_ids 6,7
> >
> > possibly:
> >
> > $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> > 6
> >
> > Moreover, I think program and map names may collide making ELFs
> > unloadable..
> It doesn't sound like a big problem, sounds like a constraint we can live
> with.
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On 11/08, Jakub Kicinski wrote: > On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote: > > This commit adds support for loading/attaching/detaching flow > > dissector program. The structure of the flow dissector program is > > assumed to be the same as in the selftests: > > > > * flow_dissector section with the main entry point > > * a bunch of tail call progs > > * a jmp_table map that is populated with the tail call progs > > Could you split the loadall changes and the flow_dissector changes into > two separate patches? Sure, will do, but let's first agree on the semantical differences of load vs loadall. So far *load* actually loads _all_ progs (via bpf_object__load), but pins only the first program. Is that what we want? I wonder whether the assumption there was that there is only single program in the object. Should we load only the first program in *load*? If we add *loadall*, then the difference would be: *load*: * loads all maps and only the first program, pins only the first program *loadall*: * loads all maps and all programs, pins everything (maps and programs) Is this the expected behavior?
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 11:16:43 +, Quentin Monnet wrote:
> > > - bpf_program__set_ifindex(prog, ifindex);
> > > if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > > + if (!prog) {
> > > + p_err("can not guess program type when loading all
> > > programs\n");
>
> No new lines in p_err(), beaks JSON.
Thanks, I'll probably remove that altogether and do auto inference of
prog_type from the section name here.
> > > + goto err_close_obj;
> > > + }
> > > +
> > > const char *sec_name = bpf_program__title(prog, false);
> > >
> > > err = libbpf_prog_type_by_name(sec_name,
> > > _type,
> > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > > goto err_close_obj;
> > > }
> > > }
> > > - bpf_program__set_type(prog, attr.prog_type);
> > > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > +
> > > + bpf_object__for_each_program(pos, obj) {
> > > + bpf_program__set_ifindex(pos, ifindex);
> > > + bpf_program__set_type(pos, attr.prog_type);
> > > + bpf_program__set_expected_attach_type(pos,
> > > + expected_attach_type);
> > > + }
> >
> > I still believe you can have programs of different types here, and be
> > able to load them. I tried it and managed to have it working fine. If no
> > type is provided from command line we can retrieve types for each
> > program from its section name. If a type is provided on the command
> > line, we can do the same, but I am not sure we should do it, or impose
> > that type for all programs instead.
>
> attr->prog_type is one per object, though. How do we set that one?
Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
version in the bpf prog?
It will probably work quite nicely for both of our options:
* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver
to require/not requqire kernel version
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 18:21:24 +, Quentin Monnet wrote:
> > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > >>> contain a dot character ('.'), which is reserved for
> > >>> future
> > >>> extensions of *bpffs*.
> > >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> > >>> {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type**
> > >>> *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev**
> > >>> *NAME*]
> > >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> > >>> + **bpftool prog load** will pin only the first bpf
> > >>> program
> > >>> + from the *OBJ*, **bpftool prog loadall** will pin all
> > >>> maps
> > >>> + and programs from the *OBJ*.
> > >>
> > >> This could be improved regarding maps: with "bpftool prog load" I think
> > >> we
> > >> also load and pin all maps, but your description implies this is only the
> > >> case with "loadall"
> > > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > > them, but we don't pin any afaict. Can you point me to the code where we
> > > pin the maps?
> > >
> >
> > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > sorry about that.
>
> Right, but I don't see much reason why prog loadall should pin maps.
It does seem convenient to have an option to pin everything, so we
don't require anything else to find the ids of the maps.
If we are pinning all progs, might as well pin the maps, why not? See
my example in the commit message, for example, where I just hard code
the expected map name. Convenient :-)
> The reason to pin program(s) is to hold some reference and to be able
> to find them. Since we have the programs pinned we should be able to
> find their maps with relative ease.
>
> $ bpftool prog show pinned /sys/fs/bpf/prog
> 7: cgroup_skb tag 2a142ef67aaad174 gpl
> loaded_at 2018-11-08T11:02:25-0800 uid 0
> xlated 296B jited 229B memlock 4096B map_ids 6,7
>
> possibly:
>
> $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> 6
>
> Moreover, I think program and map names may collide making ELFs
> unloadable..
It doesn't sound like a big problem, sounds like a constraint we can live
with.
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote: > This commit adds support for loading/attaching/detaching flow > dissector program. The structure of the flow dissector program is > assumed to be the same as in the selftests: > > * flow_dissector section with the main entry point > * a bunch of tail call progs > * a jmp_table map that is populated with the tail call progs Could you split the loadall changes and the flow_dissector changes into two separate patches?
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Thu, 8 Nov 2018 11:16:43 +, Quentin Monnet wrote:
> > - bpf_program__set_ifindex(prog, ifindex);
> > if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > + if (!prog) {
> > + p_err("can not guess program type when loading all
> > programs\n");
No new lines in p_err(), beaks JSON.
> > + goto err_close_obj;
> > + }
> > +
> > const char *sec_name = bpf_program__title(prog, false);
> >
> > err = libbpf_prog_type_by_name(sec_name, _type,
> > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > goto err_close_obj;
> > }
> > }
> > - bpf_program__set_type(prog, attr.prog_type);
> > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > +
> > + bpf_object__for_each_program(pos, obj) {
> > + bpf_program__set_ifindex(pos, ifindex);
> > + bpf_program__set_type(pos, attr.prog_type);
> > + bpf_program__set_expected_attach_type(pos,
> > + expected_attach_type);
> > + }
>
> I still believe you can have programs of different types here, and be
> able to load them. I tried it and managed to have it working fine. If no
> type is provided from command line we can retrieve types for each
> program from its section name. If a type is provided on the command
> line, we can do the same, but I am not sure we should do it, or impose
> that type for all programs instead.
attr->prog_type is one per object, though. How do we set that one?
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On Thu, 8 Nov 2018 18:21:24 +, Quentin Monnet wrote:
> >>> @@ -79,8 +82,11 @@ DESCRIPTION
> >>> contain a dot character ('.'), which is reserved for
> >>> future
> >>> extensions of *bpffs*.
> >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx**
> >>> *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
> >>> [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> >>> + **bpftool prog load** will pin only the first bpf program
> >>> + from the *OBJ*, **bpftool prog loadall** will pin all maps
> >>> + and programs from the *OBJ*.
> >>
> >> This could be improved regarding maps: with "bpftool prog load" I think we
> >> also load and pin all maps, but your description implies this is only the
> >> case with "loadall"
> > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > them, but we don't pin any afaict. Can you point me to the code where we
> > pin the maps?
> >
>
> My bad. I read "pin" but thought "load". It does not pin them indeed,
> sorry about that.
Right, but I don't see much reason why prog loadall should pin maps.
The reason to pin program(s) is to hold some reference and to be able
to find them. Since we have the programs pinned we should be able to
find their maps with relative ease.
$ bpftool prog show pinned /sys/fs/bpf/prog
7: cgroup_skb tag 2a142ef67aaad174 gpl
loaded_at 2018-11-08T11:02:25-0800 uid 0
xlated 296B jited 229B memlock 4096B map_ids 6,7
possibly:
$ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
6
Moreover, I think program and map names may collide making ELFs
unloadable..
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On 11/08, Quentin Monnet wrote:
> 2018-11-08 10:01 UTC-0800 ~ Stanislav Fomichev
> > On 11/08, Quentin Monnet wrote:
> >> Hi Stanislav, thanks for the changes! More comments below.
> > Thank you for another round of review!
> >
> >> 2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev
> >>> This commit adds support for loading/attaching/detaching flow
> >>> dissector program. The structure of the flow dissector program is
> >>> assumed to be the same as in the selftests:
> >>>
> >>> * flow_dissector section with the main entry point
> >>> * a bunch of tail call progs
> >>> * a jmp_table map that is populated with the tail call progs
> >>>
> >>> When `bpftool load` is called with a flow_dissector prog (i.e. when the
> >>> first section is flow_dissector of 'type flow_dissector' argument is
> >>> passed), we load and pin all the programs/maps. User is responsible to
> >>> construct the jump table for the tail calls.
> >>>
> >>> The last argument of `bpftool attach` is made optional for this use
> >>> case.
> >>>
> >>> Example:
> >>> bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> >>> /sys/fs/bpf/flow type flow_dissector
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 0 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/IP
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 1 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/IPV6
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 2 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/IPV6OP
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 3 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/IPV6FR
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 4 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/MPLS
> >>>
> >>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> >>> key 5 0 0 0 \
> >>> value pinned /sys/fs/bpf/flow/VLAN
> >>>
> >>> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
> >>>
> >>> Tested by using the above lines to load the prog in
> >>> the test_flow_dissector.sh selftest.
> >>>
> >>> Signed-off-by: Stanislav Fomichev
> >>> ---
> >>> .../bpftool/Documentation/bpftool-prog.rst| 36 --
> >>> tools/bpf/bpftool/bash-completion/bpftool | 6 +-
> >>> tools/bpf/bpftool/common.c| 30 ++---
> >>> tools/bpf/bpftool/main.h | 1 +
> >>> tools/bpf/bpftool/prog.c | 112 +-
> >>> 5 files changed, 126 insertions(+), 59 deletions(-)
> >>>
> >>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> >>> b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> >>> index ac4e904b10fb..0374634c3087 100644
> >>> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> >>> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> >>> @@ -15,7 +15,8 @@ SYNOPSIS
> >>> *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty**
> >>> }] | { **-f** | **--bpffs** } }
> >>> *COMMANDS* :=
> >>> - { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
> >>> **load** | **help** }
> >>> + { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
> >>> **load**
> >>> + | **loadall** | **help** }
> >>> MAP COMMANDS
> >>> =
> >>> @@ -24,9 +25,9 @@ MAP COMMANDS
> >>> | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* |
> >>> **opcodes** | **visual**}]
> >>> | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> >>> **opcodes**}]
> >>> | **bpftool** **prog pin** *PROG* *FILE*
> >>> -|**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*]
> >>> [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> >>> -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> >>> -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> >>> +|**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type**
> >>> *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> >>> +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> >>> +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >>> | **bpftool** **prog help**
> >>> |
> >>> | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> >>> @@ -39,7 +40,9 @@ MAP COMMANDS
> >>> | **cgroup/bind4** | **cgroup/bind6** |
> >>> **cgroup/post_bind4** | **cgroup/post_bind6** |
> >>> | **cgroup/connect4** | **cgroup/connect6** |
> >>> **cgroup/sendmsg4** | **cgroup/sendmsg6**
> >>> | }
> >>> -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** |
> >>> **skb_parse** }
> >>> +| *ATTACH_TYPE* := {
> >>> +|**msg_verdict** | **skb_verdict** | **skb_parse** |
> >>> **flow_dissector**
> >>> +|}
> >>> DESCRIPTION
>
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
2018-11-08 10:01 UTC-0800 ~ Stanislav Fomichev
> On 11/08, Quentin Monnet wrote:
>> Hi Stanislav, thanks for the changes! More comments below.
> Thank you for another round of review!
>
>> 2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev
>>> This commit adds support for loading/attaching/detaching flow
>>> dissector program. The structure of the flow dissector program is
>>> assumed to be the same as in the selftests:
>>>
>>> * flow_dissector section with the main entry point
>>> * a bunch of tail call progs
>>> * a jmp_table map that is populated with the tail call progs
>>>
>>> When `bpftool load` is called with a flow_dissector prog (i.e. when the
>>> first section is flow_dissector of 'type flow_dissector' argument is
>>> passed), we load and pin all the programs/maps. User is responsible to
>>> construct the jump table for the tail calls.
>>>
>>> The last argument of `bpftool attach` is made optional for this use
>>> case.
>>>
>>> Example:
>>> bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
>>> /sys/fs/bpf/flow type flow_dissector
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 0 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IP
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 1 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 2 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6OP
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 3 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6FR
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 4 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/MPLS
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 5 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/VLAN
>>>
>>> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
>>>
>>> Tested by using the above lines to load the prog in
>>> the test_flow_dissector.sh selftest.
>>>
>>> Signed-off-by: Stanislav Fomichev
>>> ---
>>> .../bpftool/Documentation/bpftool-prog.rst| 36 --
>>> tools/bpf/bpftool/bash-completion/bpftool | 6 +-
>>> tools/bpf/bpftool/common.c| 30 ++---
>>> tools/bpf/bpftool/main.h | 1 +
>>> tools/bpf/bpftool/prog.c | 112 +-
>>> 5 files changed, 126 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> index ac4e904b10fb..0374634c3087 100644
>>> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> @@ -15,7 +15,8 @@ SYNOPSIS
>>> *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
>>> **-f** | **--bpffs** } }
>>> *COMMANDS* :=
>>> - { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
>>> **load** | **help** }
>>> + { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
>>> **load**
>>> + | **loadall** | **help** }
>>> MAP COMMANDS
>>> =
>>> @@ -24,9 +25,9 @@ MAP COMMANDS
>>> | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**
>>> | **visual**}]
>>> | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
>>> **opcodes**}]
>>> | **bpftool** **prog pin** *PROG* *FILE*
>>> -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
>>> {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
>>> -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
>>> +| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
>>> [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> | **bpftool** **prog help**
>>> |
>>> | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
>>> @@ -39,7 +40,9 @@ MAP COMMANDS
>>> | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
>>> **cgroup/post_bind6** |
>>> | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
>>> | **cgroup/sendmsg6**
>>> | }
>>> -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** |
>>> **skb_parse** }
>>> +| *ATTACH_TYPE* := {
>>> +| **msg_verdict** | **skb_verdict** | **skb_parse** |
>>> **flow_dissector**
>>> +| }
>>> DESCRIPTION
>>> @@ -79,8 +82,11 @@ DESCRIPTION
>>> contain a dot character ('.'), which is reserved for future
>>> extensions of *bpffs*.
>>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx**
>>> *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> + **bpftool prog { load | loadall }**
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
On 11/08, Quentin Monnet wrote:
> Hi Stanislav, thanks for the changes! More comments below.
Thank you for another round of review!
> 2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
> >
> > When `bpftool load` is called with a flow_dissector prog (i.e. when the
> > first section is flow_dissector of 'type flow_dissector' argument is
> > passed), we load and pin all the programs/maps. User is responsible to
> > construct the jump table for the tail calls.
> >
> > The last argument of `bpftool attach` is made optional for this use
> > case.
> >
> > Example:
> > bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> > /sys/fs/bpf/flow type flow_dissector
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 0 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IP
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 1 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 2 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6OP
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 3 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6FR
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 4 0 0 0 \
> > value pinned /sys/fs/bpf/flow/MPLS
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 5 0 0 0 \
> > value pinned /sys/fs/bpf/flow/VLAN
> >
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
> >
> > Tested by using the above lines to load the prog in
> > the test_flow_dissector.sh selftest.
> >
> > Signed-off-by: Stanislav Fomichev
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst| 36 --
> > tools/bpf/bpftool/bash-completion/bpftool | 6 +-
> > tools/bpf/bpftool/common.c| 30 ++---
> > tools/bpf/bpftool/main.h | 1 +
> > tools/bpf/bpftool/prog.c | 112 +-
> > 5 files changed, 126 insertions(+), 59 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..0374634c3087 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -15,7 +15,8 @@ SYNOPSIS
> > *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
> > **-f** | **--bpffs** } }
> > *COMMANDS* :=
> > - { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
> > **load** | **help** }
> > + { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
> > **load**
> > + | **loadall** | **help** }
> > MAP COMMANDS
> > =
> > @@ -24,9 +25,9 @@ MAP COMMANDS
> > | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**
> > | **visual**}]
> > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> > **opcodes**}]
> > | **bpftool** **prog pin** *PROG* *FILE*
> > -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> > {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
> > [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > | **bpftool** **prog help**
> > |
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +40,9 @@ MAP COMMANDS
> > | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
> > **cgroup/post_bind6** |
> > | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
> > | **cgroup/sendmsg6**
> > | }
> > -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** |
> > **skb_parse** }
> > +| *ATTACH_TYPE* := {
> > +| **msg_verdict** | **skb_verdict** | **skb_parse** |
> > **flow_dissector**
> > +| }
> > DESCRIPTION
> > @@ -79,8 +82,11 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> > - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx**
> > *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
> >
Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
Hi Stanislav, thanks for the changes! More comments below.
2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev
This commit adds support for loading/attaching/detaching flow
dissector program. The structure of the flow dissector program is
assumed to be the same as in the selftests:
* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs
When `bpftool load` is called with a flow_dissector prog (i.e. when the
first section is flow_dissector of 'type flow_dissector' argument is
passed), we load and pin all the programs/maps. User is responsible to
construct the jump table for the tail calls.
The last argument of `bpftool attach` is made optional for this use
case.
Example:
bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
/sys/fs/bpf/flow type flow_dissector
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 1 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 2 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6OP
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 3 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6FR
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 4 0 0 0 \
value pinned /sys/fs/bpf/flow/MPLS
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 5 0 0 0 \
value pinned /sys/fs/bpf/flow/VLAN
bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
Tested by using the above lines to load the prog in
the test_flow_dissector.sh selftest.
Signed-off-by: Stanislav Fomichev
---
.../bpftool/Documentation/bpftool-prog.rst| 36 --
tools/bpf/bpftool/bash-completion/bpftool | 6 +-
tools/bpf/bpftool/common.c| 30 ++---
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 112 +-
5 files changed, 126 insertions(+), 59 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index ac4e904b10fb..0374634c3087 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -15,7 +15,8 @@ SYNOPSIS
*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
**-f** | **--bpffs** } }
*COMMANDS* :=
- { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
**load** | **help** }
+ { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
**load**
+ | **loadall** | **help** }
MAP COMMANDS
=
@@ -24,9 +25,9 @@ MAP COMMANDS
| **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**
| **visual**}]
| **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
**opcodes**}]
| **bpftool** **prog pin** *PROG* *FILE*
-| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
{**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
-| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -39,7 +40,9 @@ MAP COMMANDS
| **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
**cgroup/post_bind6** |
| **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
| **cgroup/sendmsg6**
| }
-| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
+| *ATTACH_TYPE* := {
+| **msg_verdict** | **skb_verdict** | **skb_parse** |
**flow_dissector**
+| }
DESCRIPTION
@@ -79,8 +82,11 @@ DESCRIPTION
contain a dot character ('.'), which is reserved for future
extensions of *bpffs*.
- **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+ **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Load bpf program from binary *OBJ* and pin as *FILE*.
+ **bpftool prog load** will pin only the first bpf program
+ from the *OBJ*, **bpftool prog loadall** will pin all maps
+ and programs from the *OBJ*.
This could be improved regarding maps: with "bpftool prog load" I think
we also load
[PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
This commit adds support for loading/attaching/detaching flow
dissector program. The structure of the flow dissector program is
assumed to be the same as in the selftests:
* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs
When `bpftool load` is called with a flow_dissector prog (i.e. when the
first section is flow_dissector of 'type flow_dissector' argument is
passed), we load and pin all the programs/maps. User is responsible to
construct the jump table for the tail calls.
The last argument of `bpftool attach` is made optional for this use
case.
Example:
bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
/sys/fs/bpf/flow type flow_dissector
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 1 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 2 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6OP
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 3 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6FR
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 4 0 0 0 \
value pinned /sys/fs/bpf/flow/MPLS
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 5 0 0 0 \
value pinned /sys/fs/bpf/flow/VLAN
bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
Tested by using the above lines to load the prog in
the test_flow_dissector.sh selftest.
Signed-off-by: Stanislav Fomichev
---
.../bpftool/Documentation/bpftool-prog.rst| 36 --
tools/bpf/bpftool/bash-completion/bpftool | 6 +-
tools/bpf/bpftool/common.c| 30 ++---
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 112 +-
5 files changed, 126 insertions(+), 59 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index ac4e904b10fb..0374634c3087 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -15,7 +15,8 @@ SYNOPSIS
*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
**-f** | **--bpffs** } }
*COMMANDS* :=
- { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
**load** | **help** }
+ { **show** | **list** | **dump xlated** | **dump jited** | **pin** |
**load**
+ | **loadall** | **help** }
MAP COMMANDS
=
@@ -24,9 +25,9 @@ MAP COMMANDS
| **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**
| **visual**}]
| **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
**opcodes**}]
| **bpftool** **prog pin** *PROG* *FILE*
-| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
{**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
-| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -39,7 +40,9 @@ MAP COMMANDS
| **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
**cgroup/post_bind6** |
| **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
| **cgroup/sendmsg6**
| }
-| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
+| *ATTACH_TYPE* := {
+| **msg_verdict** | **skb_verdict** | **skb_parse** |
**flow_dissector**
+| }
DESCRIPTION
@@ -79,8 +82,11 @@ DESCRIPTION
contain a dot character ('.'), which is reserved for future
extensions of *bpffs*.
- **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx**
*IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+ **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Load bpf program from binary *OBJ* and pin as *FILE*.
+ **bpftool prog load** will pin only the first bpf program
+ from the *OBJ*, **bpftool prog loadall** will pin all maps
+ and programs from the *OBJ*.
**type** is optional, if not specified program type will be
inferred from section names.
By default bpftool will create new maps as declared in the ELF
@@ -97,13
