Date:   Wed, 23 Jan 2019 15:11:09 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Cc:     ast@...nel.org, netdev@...r.kernel.org,
        jakub.kicinski@...ronome.com, brouer@...hat.com,
        quentin.monnet@...ronome.com
Subject: Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a
 prog via index

On 01/23/2019 02:41 PM, Maciej Fijalkowski wrote:
> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@...earbox.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@...il.com>
>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.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;
>>>   
>>
> 

Powered by blists - more mailing lists