[bpf-next,v2,2/8] libbpf: Add a helper for retrieving a prog via index

xdp_redirect_cpu has a 6 different XDP programs that can be attached to
network interface. This sample has a option --prognum that allows user
for specifying which particular program from a given set will be
attached to network interface.
In order to make it easier when converting the mentioned sample to
libbpf usage, add a function to libbpf that will return program's fd for
a given index.

Note that there is already a bpf_object__find_prog_by_idx, which could
be exported and might be used for that purpose, but it operates on the
number of ELF section and here we need an index from a programs array
within the bpf_object.

Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/lib/bpf/libbpf.c   | 8 ++++++++
 tools/lib/bpf/libbpf.h   | 3 +++
 tools/lib/bpf/libbpf.map | 1 +
 3 files changed, 12 insertions(+)

On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> network interface. This sample has a option --prognum that allows user
> for specifying which particular program from a given set will be
> attached to network interface.
> In order to make it easier when converting the mentioned sample to
> libbpf usage, add a function to libbpf that will return program's fd for
> a given index.
> 
> Note that there is already a bpf_object__find_prog_by_idx, which could
> be exported and might be used for that purpose, but it operates on the
> number of ELF section and here we need an index from a programs array
> within the bpf_object.

Series in general looks good to me. Few minor comments, mainly in relation
to the need for libbpf extensions.

Would it not be a better interface to the user to instead choose the prog
based on section name and then retrieve it via bpf_object__find_program_by_title()
instead of prognum (which feels less user friendly) at least?

> Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  tools/lib/bpf/libbpf.c   | 8 ++++++++
>  tools/lib/bpf/libbpf.h   | 3 +++
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 12 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index dc838bea403f..21c84d0f6128 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>  	return err;
>  }
>  
> +int
> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
> +{
> +	if (idx >= 0 && idx < obj->nr_programs)
> +		return bpf_program__fd(&obj->programs[idx]);
> +	return -ENOENT;
> +}
> +
>  static struct bpf_program *
>  bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
>  {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 7f10d36abdde..ca1b381cb3ad 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
>  LIBBPF_API struct bpf_program *
>  bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
>  
> +LIBBPF_API int
> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
> +
>  LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
>  #define bpf_object__for_each_safe(pos, tmp)			\
>  	for ((pos) = bpf_object__next(NULL),		\
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 7c59e4f64082..871d2fc07150 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
>  LIBBPF_0.0.2 {
>  	global:
>  		bpf_object__find_map_fd_by_name;
> +		bpf_object__get_prog_fd_by_num;
>  } LIBBPF_0.0.1;
>

On Wed, 23 Jan 2019 11:41:11 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> > xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> > network interface. This sample has a option --prognum that allows user
> > for specifying which particular program from a given set will be
> > attached to network interface.
> > In order to make it easier when converting the mentioned sample to
> > libbpf usage, add a function to libbpf that will return program's fd for
> > a given index.
> > 
> > Note that there is already a bpf_object__find_prog_by_idx, which could
> > be exported and might be used for that purpose, but it operates on the
> > number of ELF section and here we need an index from a programs array
> > within the bpf_object.  
> 
> Series in general looks good to me. Few minor comments, mainly in relation
> to the need for libbpf extensions.
> 
> Would it not be a better interface to the user to instead choose the prog
> based on section name and then retrieve it via bpf_object__find_program_by_title()
> instead of prognum (which feels less user friendly) at least?
> 

I couldn't decide which one from:
* adding a libbpf helper
* changing the xdp_redirect_cpu behaviour
would be more invasive when I was converting this sample to libbpf support.

Your suggestion sounds good, but I'm wondering about the actual implementation.
I suppose that we would choose the program via command line. Some program
section names in this sample are a bit long and it might be irritating for
user to type in for example "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we
can live with this. In case of typo and program being not found it would be
good to print out the section names to choose from in usage().

> > Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  tools/lib/bpf/libbpf.c   | 8 ++++++++
> >  tools/lib/bpf/libbpf.h   | 3 +++
> >  tools/lib/bpf/libbpf.map | 1 +
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index dc838bea403f..21c84d0f6128 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> >  	return err;
> >  }
> >  
> > +int
> > +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
> > +{
> > +	if (idx >= 0 && idx < obj->nr_programs)
> > +		return bpf_program__fd(&obj->programs[idx]);
> > +	return -ENOENT;
> > +}
> > +
> >  static struct bpf_program *
> >  bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
> >  {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 7f10d36abdde..ca1b381cb3ad 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
> >  LIBBPF_API struct bpf_program *
> >  bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
> >  
> > +LIBBPF_API int
> > +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
> > +
> >  LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
> >  #define bpf_object__for_each_safe(pos, tmp)			\
> >  	for ((pos) = bpf_object__next(NULL),		\
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 7c59e4f64082..871d2fc07150 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
> >  LIBBPF_0.0.2 {
> >  	global:
> >  		bpf_object__find_map_fd_by_name;
> > +		bpf_object__get_prog_fd_by_num;
> >  } LIBBPF_0.0.1;
> >   
>

On 01/23/2019 02:41 PM, Maciej Fijalkowski wrote:
> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
>>> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
>>> network interface. This sample has a option --prognum that allows user
>>> for specifying which particular program from a given set will be
>>> attached to network interface.
>>> In order to make it easier when converting the mentioned sample to
>>> libbpf usage, add a function to libbpf that will return program's fd for
>>> a given index.
>>>
>>> Note that there is already a bpf_object__find_prog_by_idx, which could
>>> be exported and might be used for that purpose, but it operates on the
>>> number of ELF section and here we need an index from a programs array
>>> within the bpf_object.  
>>
>> Series in general looks good to me. Few minor comments, mainly in relation
>> to the need for libbpf extensions.
>>
>> Would it not be a better interface to the user to instead choose the prog
>> based on section name and then retrieve it via bpf_object__find_program_by_title()
>> instead of prognum (which feels less user friendly) at least?
> 
> I couldn't decide which one from:
> * adding a libbpf helper
> * changing the xdp_redirect_cpu behaviour
> would be more invasive when I was converting this sample to libbpf support.
> 
> Your suggestion sounds good, but I'm wondering about the actual implementation.
> I suppose that we would choose the program via command line. Some program

Yes, selection for loading prog could be done through that.

> section names in this sample are a bit long and it might be irritating for
> user to type in for example "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we

I think that's an implementation detail of this specific sample. For the
prog number, the user would need to first look up the source and figure out
the prog number correlation to the name that should be loaded which is a
bit ugly (unless I missed something). So we might as well make it easier
and allow to choose by name.

Figuring out which programs are available in an object file is a separate
tooling issue, imho. readelf is one way but not optimal, and it would be
a nice addition to bpftool as brought up at plumbers [0] to dump the contents
of a bpf obj file. Latter might also be nice to enrich with BTF info when
present (e.g. types, line annotation etc) so this would be much more powerful
than plain introspection via readelf.

 [0] http://vger.kernel.org/lpc-bpf2018.html#session-6

> can live with this. In case of typo and program being not found it would be
> good to print out the section names to choose from in usage().

Agree, might be nice as well via libbpf.

>>> Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>>  tools/lib/bpf/libbpf.c   | 8 ++++++++
>>>  tools/lib/bpf/libbpf.h   | 3 +++
>>>  tools/lib/bpf/libbpf.map | 1 +
>>>  3 files changed, 12 insertions(+)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index dc838bea403f..21c84d0f6128 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>>>  	return err;
>>>  }
>>>  
>>> +int
>>> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
>>> +{
>>> +	if (idx >= 0 && idx < obj->nr_programs)
>>> +		return bpf_program__fd(&obj->programs[idx]);
>>> +	return -ENOENT;
>>> +}
>>> +
>>>  static struct bpf_program *
>>>  bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
>>>  {
>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>> index 7f10d36abdde..ca1b381cb3ad 100644
>>> --- a/tools/lib/bpf/libbpf.h
>>> +++ b/tools/lib/bpf/libbpf.h
>>> @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
>>>  LIBBPF_API struct bpf_program *
>>>  bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
>>>  
>>> +LIBBPF_API int
>>> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
>>> +
>>>  LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
>>>  #define bpf_object__for_each_safe(pos, tmp)			\
>>>  	for ((pos) = bpf_object__next(NULL),		\
>>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>>> index 7c59e4f64082..871d2fc07150 100644
>>> --- a/tools/lib/bpf/libbpf.map
>>> +++ b/tools/lib/bpf/libbpf.map
>>> @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
>>>  LIBBPF_0.0.2 {
>>>  	global:
>>>  		bpf_object__find_map_fd_by_name;
>>> +		bpf_object__get_prog_fd_by_num;
>>>  } LIBBPF_0.0.1;
>>>   
>>
>

On Wed, 23 Jan 2019 14:41:59 +0100
Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote:

> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> > On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:  
> > > xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> > > network interface. This sample has a option --prognum that allows user
> > > for specifying which particular program from a given set will be
> > > attached to network interface.
> > > In order to make it easier when converting the mentioned sample to
> > > libbpf usage, add a function to libbpf that will return program's fd for
> > > a given index.
> > > 
> > > Note that there is already a bpf_object__find_prog_by_idx, which could
> > > be exported and might be used for that purpose, but it operates on the
> > > number of ELF section and here we need an index from a programs array
> > > within the bpf_object.    
> > 
> > Series in general looks good to me. Few minor comments, mainly in relation
> > to the need for libbpf extensions.
> > 
> > Would it not be a better interface to the user to instead choose the prog
> > based on section name and then retrieve it via bpf_object__find_program_by_title()
> > instead of prognum (which feels less user friendly) at least?
> >   
> 
> I couldn't decide which one from:
> * adding a libbpf helper
> * changing the xdp_redirect_cpu behaviour
>
> would be more invasive when I was converting this sample to libbpf
> support.
> 
> Your suggestion sounds good, but I'm wondering about the actual
> implementation. I suppose that we would choose the program via
> command line. Some program section names in this sample are a bit
> long and it might be irritating for user to type in for example
> "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we can live with this.
> In case of typo and program being not found it would be good to print
> out the section names to choose from in usage().

Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
option.  I would prefer if this was indeed converted to selecting the
program based on the name in the _kern.c file, instead of a number.

Listing the avail programs will be helpful, and you can also
shorten/rename the program names.

For easy of use, consider doing like 'ip' tool from[1] iproute2, and
find the first best matching string.  Copy pasted code below signature
for inspiration.

On 01/23/2019 03:24 PM, Jesper Dangaard Brouer wrote:
> On Wed, 23 Jan 2019 14:41:59 +0100
> Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote:
> 
>> On Wed, 23 Jan 2019 11:41:11 +0100
>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>>> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:  
>>>> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
>>>> network interface. This sample has a option --prognum that allows user
>>>> for specifying which particular program from a given set will be
>>>> attached to network interface.
>>>> In order to make it easier when converting the mentioned sample to
>>>> libbpf usage, add a function to libbpf that will return program's fd for
>>>> a given index.
>>>>
>>>> Note that there is already a bpf_object__find_prog_by_idx, which could
>>>> be exported and might be used for that purpose, but it operates on the
>>>> number of ELF section and here we need an index from a programs array
>>>> within the bpf_object.    
>>>
>>> Series in general looks good to me. Few minor comments, mainly in relation
>>> to the need for libbpf extensions.
>>>
>>> Would it not be a better interface to the user to instead choose the prog
>>> based on section name and then retrieve it via bpf_object__find_program_by_title()
>>> instead of prognum (which feels less user friendly) at least?
>>
>> I couldn't decide which one from:
>> * adding a libbpf helper
>> * changing the xdp_redirect_cpu behaviour
>>
>> would be more invasive when I was converting this sample to libbpf
>> support.
>>
>> Your suggestion sounds good, but I'm wondering about the actual
>> implementation. I suppose that we would choose the program via
>> command line. Some program section names in this sample are a bit
>> long and it might be irritating for user to type in for example
>> "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we can live with this.
>> In case of typo and program being not found it would be good to print
>> out the section names to choose from in usage().
> 
> Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
> option.  I would prefer if this was indeed converted to selecting the
> program based on the name in the _kern.c file, instead of a number.

Given this is BPF sample code, there is no need to deprecate, lets just
remove --prognum option and add a new one for selection by name.

> Listing the avail programs will be helpful, and you can also
> shorten/rename the program names.
> 
> For easy of use, consider doing like 'ip' tool from[1] iproute2, and
> find the first best matching string.  Copy pasted code below signature
> for inspiration.

On Thu, 24 Jan 2019 12:56:36 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> > Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
> > option.  I would prefer if this was indeed converted to selecting the
> > program based on the name in the _kern.c file, instead of a number.  
> 
> Given this is BPF sample code, there is no need to deprecate, lets just
> remove --prognum option and add a new one for selection by name.

I agree, just remove  --prognum option.

On Thu, 24 Jan 2019 13:09:49 +0100
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Thu, 24 Jan 2019 12:56:36 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> > > Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
> > > option.  I would prefer if this was indeed converted to selecting the
> > > program based on the name in the _kern.c file, instead of a number.  
> > 
> > Given this is BPF sample code, there is no need to deprecate, lets just
> > remove --prognum option and add a new one for selection by name.
> 
> I agree, just remove  --prognum option.
> 
Actually we can convert it to --progname and use it for providing the section
name of program to be loaded.

On Wed, 23 Jan 2019 15:11:09 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> > can live with this. In case of typo and program being not found it would be
> > good to print out the section names to choose from in usage().
> 
> Agree, might be nice as well via libbpf.

Would it be acceptable to print them from within libbpf? Something as trivial
as:
void
bpf_object__print_type_sections(struct bpf_object *obj,
				enum bpf_prog_type type)
{
	struct bpf_program *prog;

	bpf_object__for_each_program(prog, obj) {
		if (bpf_program__is_type(prog, type))
			pr_info("section %s\n", prog->section_name);
	}
}
Or do we want to add a accessor for section_name and go through programs in
application?

On Thu, 24 Jan 2019 19:27:35 +0100, Maciej Fijałkowski wrote:
> > > can live with this. In case of typo and program being not found it would be
> > > good to print out the section names to choose from in usage().  
> > 
> > Agree, might be nice as well via libbpf.  
> 
> Would it be acceptable to print them from within libbpf? Something as trivial
> as:
> void
> bpf_object__print_type_sections(struct bpf_object *obj,
> 				enum bpf_prog_type type)
> {
> 	struct bpf_program *prog;
> 
> 	bpf_object__for_each_program(prog, obj) {
> 		if (bpf_program__is_type(prog, type))
> 			pr_info("section %s\n", prog->section_name);
> 	}
> }

Better not.

> Or do we want to add a accessor for section_name and go through programs in
> application?

bpf_program__title(), no?