Message-ID: <581A0155.40302@iogearbox.net>
Date:   Wed, 02 Nov 2016 16:08:05 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Joe Stringer <joe@....org>
CC:     netdev <netdev@...r.kernel.org>, wangnan0@...wei.com, ast@...com
Subject: Re: [PATCH net-next 3/3] tools lib bpf: Sync bpf_map_def with tc

On 11/02/2016 03:12 PM, Daniel Borkmann wrote:
> On 11/02/2016 05:09 AM, Joe Stringer wrote:
>> On 1 November 2016 at 20:09, Daniel Borkmann <daniel@...earbox.net> wrote:
>>> On 10/31/2016 07:39 PM, Joe Stringer wrote:
>>>>
>>>> TC uses a slightly different map layout in its ELFs. Update libbpf to
>>>> use the same definition so that ELFs may be built using libbpf and
>>>> loaded using tc.
>>>>
>>>> Signed-off-by: Joe Stringer <joe@....org>
>>>> ---
>>>>    tools/lib/bpf/libbpf.h | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>>> index dd7a513efb10..ea70c2744f8c 100644
>>>> --- a/tools/lib/bpf/libbpf.h
>>>> +++ b/tools/lib/bpf/libbpf.h
>>>> @@ -181,10 +181,13 @@ bool bpf_program__is_kprobe(struct bpf_program
>>>> *prog);
>>>>     * and will be treated as an error due to -Werror.
>>>>     */
>>>>    struct bpf_map_def {
>>>> -       unsigned int type;
>>>> -       unsigned int key_size;
>>>> -       unsigned int value_size;
>>>> -       unsigned int max_entries;
>>>> +       uint32_t type;
>>>> +       uint32_t key_size;
>>>> +       uint32_t value_size;
>>>> +       uint32_t max_entries;
>>>> +       uint32_t flags;
>>>> +       uint32_t id;
>>>> +       uint32_t pinning;
>>>>    };
>>>>
>>>>    /*
>>>
>>> I think the problem is that this would break existing obj files that have
>>> been compiled with the current struct bpf_map_def (besides libbpf not having
>>> a use for the last two members right now).
>>
>> Right, this is a problem. I wasn't sure whether libbpf was yet at a
>> stage where it tries to retain compatibility with binaries compiled
>> against older kernels.
>>
>>> For tc, we have a refactoring of the tc_bpf.c bits that generalizes them so
>>> we can move these bits into iproute2 lib part and add new BPF types really
>>> easily. What I did along with that is to implement a map compat mode, where
>>> it detects the size of struct bpf_elf_map (or however you want to name it)
>>> from the obj file and fixes up the missing members with some reasonable
>>> default,
>>> so these programs can still be loaded. Thus, the sample code using the
>>> current
>>> struct bpf_map_def will then work with tc as well. (I'll post the iproute2
>>> patch early next week.)
>>
>> Are you encoding the number of maps into the start of the maps section
>> in the ELF then using that to divide out and determine the size?
>
> No. It's walking the symbol table and simply counts the number of symbols
> that are present in the maps section with correct st_info attribution. It
> works because that section name is fixed ABI for all cases and really per
> definition only map structs are present there. The minimum attributes which
> are allowed to be loaded are type, key_size, value_size and max_entries.
>
>> I look forward to your patches. Maybe if TC is more tolerant of other
>> map definition sizes then this patch is less relevant.

Just a thought (not really related to your set though): Perhaps it makes sense
for the bpflib, to also add an option where the user passes a callback to parse
the map section itself if something else than struct bpf_map_def is expected in
the obj, and then have a way to access the meta data along with the fd via lib
api later on. Perhaps that would also include another callback passed to the
lib that would take care to invoke bpf(2) for map creation, which could be useful
if some custom interaction with bpffs is desired.

Powered by blists - more mailing lists