Message-ID: <20210511230505.z3rdnppplk3v3jce@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 11 May 2021 16:05:05 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Lorenz Bauer <lmb@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: bpf libraries and static variables. Was: [PATCH v2 bpf-next 2/6]
 libbpf: rename static variables during linking

On Tue, May 11, 2021 at 11:59:01AM -0700, Andrii Nakryiko wrote:
> >
> > If I understood what folks were saying no one is excited about namespaces in C.
> > So probably #3 is out and sounds like 1 is prefered?
> > So don't emit statics ?
> >
> 
> I'm in favor of not emitting statics. They just seem to cause more
> problems than providing any extra benefits. Given it's trivial to just
> use globals instead and global vs static is already an explicit signal
> of what has to be in BPF skeleton and what's not. See my RFC about
> __internal + __hidden semantics, but even if we supported nothing but
> STV_DEFAULT globals wouldn't be horrible. Clearly we'd expect users to
> just not mess with BPF library's internal state with not way to
> enforce that, so I'd still like to have some enforceability.

I'm glad that with Daniel and Lorenz we have a strong consensus here.
So I've applied first 6 patches from your RFC that stop exposing static
in skeleton and fix tests.
I'm only not 100% sure whether
commit 31332ccb7562 ("bpftool: Stop emitting static variables in BPF skeleton")
is enough to skip them reliably.
I think 'char __pad[N]' logic would do the rigth thing when
statics and globals are interleaved, but would be good to have a test.
In one .o file clang emits all globals first and then all statics,
so even without __pad it would have been enough,
but I don't know how .o-s with statics+global look after static linker combines them.

I skipped patch 7, since without llvm part it cannot be used and it's
not clear yet whether llvm will ever be able to emit __internal.

> So my proposal is to allow having a special "library identifier"
> variable, e.g., something like:
> 
> SEC(".lib") static char[] lib_name = "my_fancy_lib"; (let's not
> bikeshed naming for now)

without 'static' probably? since license and version are without?

and at will be optional (mostly ignored by toolchain) for libs that
don't need sub-skeleton and mandatory for sub-skeleton?

> With such library identifiers, BPF static linker will:
>   1) enforce uniqueness of library names when linking together
> multiple libraries

you're not proposing for lib name to do namespacing of globals, right?
Only to indicate that liblru.o and libct.o (as normal elf files)
are bpf libraries as can be seen in their 'lib_name' strings
instead of regular .o files.
(that would be a definition of bpf library).
So linker can rely on explicit library names given by users in .bpf.c
(and corresponding dependency on sub-skel) instead of relying
on file names?
If so, I agree that it's necessary.
Such 'char lib_name[]' is probably better than elf note.

>   2) append zero-size markers to the very beginning and the very end
> of each BPF library's DATASECS, something like
> 
> DATASEC .data:
> 
>    void *___my_fancy_lib__start[0];
>    /* here goes .data variables */
>    void *___my_fancy_lib__end[0];
> 
> And so on for each DATASEC. What those markers provide? Two things:
> 
> 1). It makes it much easier for sub-skeleton to find where a
> particular BPF library's .data/.rodata/.bss starts within the final
> BPF application's  .data/.rodata/.bss section. All that without
> storing local BTF and doing type comparisons. Only a simple loop over
> DATASECs and its variables is needed.

indeed. some lib name or rather sub-skeleton name is needed.
Since progs can have extern funcs in the lib I see no clean way to
reliably split prog loading between main skeleton and sub-skeletons.
Meaning that prog loading and map creation can only be done
by the main skeleton.
After that is done and mmap-ing of data/rodata/bss is done
the main skeleton will init sub-skeleton with offsets to their
corresponding data based on these offsets?
I think that will work for light skel.
I don't see a use case for __end marker yet, but I guess it's good
for consistency.
rodata init is tricky.
Since the main skel and sub-skels will init their parts independently.
But I think it can be managed.

> 2). (optionally) we can exclude everything between ___<libname>__start
> and ___<libname>__end from BPF application's skeleton.

So that's leaning towards namespacing ideas?
The lib_name doesn't hide any names and globals will conflict during
the linking as usual.
But with this optional hiding (inside .bpf.c it will have special name?)
the partial namespacing can happen. And the lib can hide the stuff
from its users.
The concept is nice, but lib scope maybe too big.

> It's a pretty long email already and there are a lot things to unpack,
> and still more nuances to discuss. So I'll put up BPF static linking +
> BPF libraries topic for this week's BPF office hours for anyone
> interested to join the live discussion. It would hopefully allow
> everyone to get more up to speed and onto the same page on this topic.
> But still curious WDYT?

Sounds great to me. I hope more folks can join the discussion.

Powered by blists - more mailing lists