Message-ID: <87h81v2s0i.fsf@toke.dk>
Date:   Fri, 20 Dec 2019 11:47:09 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

Andrii Nakryiko <andrii.nakryiko@...il.com> writes:

> On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@...hat.com>
>>
>> This adds support for resolving function externs to libbpf, with a new API
>> to resolve external function calls by static linking at load-time. The API
>> for this requires the caller to supply the object files containing the
>> target functions, and to specify an explicit mapping between extern
>> function names in the calling program, and function names in the target
>> object file. This is to support the XDP multi-prog case, where the
>> dispatcher program may not necessarily have control over function names in
>> the target programs, so simple function name resolution can't be used.
>>
>> The target object files must be loaded into the kernel before the calling
>> program, to ensure all relocations are done on the target functions, so we
>> can just copy over the instructions.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
>> ---
>
> A bunch of this code will change after you update to latest Clang with
> proper type info for extern functions. E.g., there shouldn't be any
> size/alignment for BTF_KIND_FUNC_PROTO, it's illegal. But that
> Yonghong already mentioned.

Yup, that fix should be helpful.

> As for the overall approach. I think doing static linking outside of
> bpf_object opening/loading is cleaner approach. If we introduce
> bpf_linker concept/object and have someting like
> bpf_linked__new(options) + a sequence of
> bpf_linker__add_object(bpf_object) + final bpf_linker__link(), which
> will produce usable bpf_object, as if bpf_object__open() was just
> called, it will be better and will allow quite a lot of flexibility in
> how we do things, without cluttering bpf_object API itself.

Hmm, that's not a bad idea, actually. To me it would make more sense
with an API like:

linker = bpf_linker__new(bpf_prog, opts); // start linking of bpf_prog
bpf_linker__resolve_func_static(linker, "func1", other_obj, "tgt_funcname");
bpf_linker__resolve_func_dynamic(linker, "func1", prog_fd);

new_obj = bpf_linker__finish();

I'll look into that when I pick this up again after the holidays :)

> Additionally, we can even have bpf_linker__write_file() to emit a
> final ELF file with statically linked object, which can then be loaded
> through bpf_object__open_file (we can do the same for in-memory
> buffer, of course). You can imagine LLC some day using libbpf to do
> actual linking of BPF .o files into a final BPF executable/object
> file, just like you expect it to do for non-BPF object files. WDYT?

Hmm, yeah, I don't see why we shouldn't be able to get there in the
future. Don't really have an opinion on whether it would be useful for
LLC to pull in the libbpf linker functions, though; maybe? :)

> Additionally, and seems you already realized that as well (judging by
> FIXMEs), we'll need to merge those individual objects' BTFs and
> deduplicate them, so that they form coherent set of types.

Yes, will have to look into this; any reason the existing de-duplication
code can't be reused here? I.e., could we just copy over all the BTF
info from the target object, and then run the de-duplication logic to
narrow it back down to one coherent set? Or would something different be
needed?

> Adjusting line info/func info is mandatory as well.

Yes, seems just copying it was not enough; will happily admit I was just
cargo-culting that bit ;) Guess I'll need to go figure out how line/func
info is actually supposed to work...

> Another thing we should think through is sharing maps. With
> BTF-defined maps, it should be pretty easy to have declaration vs
> definiton of maps. E.g.,
>
> prog_a.c:
>
> struct {
>     __uint(type, BPF_MAP_TYPE_ARRAY);
>     __uint(max_entries, 123);
>     ... and so on, complete definition
> } my_map SEC(".maps");
>
> prog_b.c:
>
> extern struct {
>     ... here we can discuss which pieces are necessary/allowed,
> potentially all (and they all should match, of course) ...
> } my_map SEC(".maps");
>
> prog_b.c won't create a new map, it will just use my_map from
> prog_a.c.

Ah, yes, that could be interesting. I guess we could use the same
"should I re-use" logic as we're doing for pinning map reuse (and
augment that to consider BTF as well in the process).

Is the existing llvm support sufficient to just mark a map struct as
'extern', or would something new be needed? Would it be enough to just
augment the bpf_object__init_user_btf_maps() to look for extern symbols?

> I might be missing something else as well, but those are the top things, IMO.

Right; let's see if that is not enough to at least get to an MVP for
linking. We can always improve things later :)

> I hope this is helpful.

Certainly! Thanks for the feedback!

-Toke

Powered by blists - more mailing lists