On Thu, Sep 3, 2020 at 6:29 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Sep 02, 2020 at 07:31:33PM -0700, Andrii Nakryiko wrote:
> > On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> > >
> > > From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > >
> > > The patch adds a simple wrapper bpf_prog_bind_map around the syscall.
> > > And when using libbpf to load a program, it will probe the kernel for
> > > the support of this syscall, and scan for the .metadata ELF section
> > > and load it as an internal map like a .data section.
> > >
> > > In the case that kernel supports the BPF_PROG_BIND_MAP syscall and
> > > a .metadata section exists, the map will be explicitly bound to
> > > the program via the syscall immediately after program is loaded.
> > > -EEXIST is ignored for this syscall.
> >
> > Here is the question I have. How important is it that all this
> > metadata is in a separate map? What if libbpf just PROG_BIND_MAP all
> > the maps inside a single BPF .o file to all BPF programs in that file?
> > Including ARRAY maps created for .data, .rodata and .bss, even if the
> > BPF program doesn't use any of the global variables? If it's too
> > extreme, we could do it only for global data maps, leaving explicit
> > map definitions in SEC(".maps") alone. Would that be terrible?
> > Conceptually it makes sense, because when you program in user-space,
> > you expect global variables to be there, even if you don't reference
> > it directly, right? The only downside is that you won't have a special
> > ".metadata" map, rather it will be part of ".rodata" one.
>
> That's an interesting idea.
> Indeed. If we have BPF_PROG_BIND_MAP command why do we need to create
> another map that behaves exactly like .rodata but has a different name?
That was exactly my thought when I re-read this patch set :)
> Wouldn't it be better to identify metadata elements some other way?
> Like by common prefix/suffix name of the variables or
> via grouping them under one structure with standard prefix?
> Like:
> struct bpf_prog_metadata_blahblah {
> char compiler_name[];
> int my_internal_prog_version;
> } = { .compiler_name[] = "clang v.12", ...};
>
> In the past we did this hack for 'version' and for 'license',
> but we did it because we didn't have BTF and there was no other way
> to determine the boundaries.
> I think libbpf can and should support multiple rodata sections with
Yep, that's coming, we already have a pretty common .rodata.str1.1
section emitted by Clang for some cases, which libbpf currently
ignores, but that should change. Creating a separate map for all such
small sections seems excessive, so my plan is to combine them and
their BTFs into one, as you assumed below.
> arbitrary names, but hardcoding one specific ".metadata" name?
> Hmm. Let's think through the implications.
> Multiple .o support and static linking is coming soon.
> When two .o-s with multiple bpf progs are statically linked libbpf
> won't have any choice but to merge them together under single
> ".metadata" section and single map that will be BPF_PROG_BIND_MAP-ed
> to different progs. Meaning that metadata applies to final elf file
> after linking. It's _not_ per program metadata.
Right, exactly.
> May be we should talk about problem statement and goals.
> Do we actually need metadata per program or metadata per single .o
> or metadata per final .o with multiple .o linked together?
> What is this metadata?
Yep, that's a very valid question. I've also CC'ed Andrey.
> If it's just unreferenced by program read only data then no special names or
> prefixes are needed. We can introduce BPF_PROG_BIND_MAP to bind any map to any
> program and it would be up to tooling to decide the meaning of the data in the
> map. For example, bpftool can choose to print all variables from all read only
> maps that match "bpf_metadata_" prefix, but it will be bpftool convention only
> and not hard coded in libbpf.
Agree as well. It feels a bit odd for libbpf to handle ".metadata"
specially, given libbpf itself doesn't care about its contents at all.
So thanks for bringing this up, I think this is an important discussion to have.
