Message-ID: <CAEf4Bzb+2xpacSmxF_NqENu8EtP3Z8GOv=amL_e1Q94=YPVhLA@mail.gmail.com>
Date:   Tue, 8 Sep 2020 11:24:21 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v3 3/8] libbpf: Add BPF_PROG_BIND_MAP syscall and
 use it on .metadata section

On Tue, Sep 8, 2020 at 10:45 AM Andrey Ignatov <rdna@...com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> [Fri, 2020-09-04 16:19 -0700]:
> > On Thu, Sep 3, 2020 at 6:29 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> 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@...gle.com> wrote:
> > > > >
> > > > > From: YiFei Zhu <zhuyifei@...gle.com>
> > > > >
> > > > > 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.
>
> From my side the problem statement is to be able to save a bunch of
> metadata fields per BPF object file (I don't distinguish "final .o" and
> "multiple .o linked together" since we have only the former in prod).

We don't *yet*. But reading below, you have the entire BPF application
(i.e., a collection of maps, variables and programs) in mind, not
specifically single .c file compiled into a single .o file. So
everything works out, I think.

>
> Specifically things like oncall team who owns the programs in the object
> (the most important info), build info (repository revision, build commit
> time, build time), etc. The plan is to integrate it with build system
> and be able to quickly identify source code and point of contact for any
> particular program.
>
> All these things are always the same for all programs in one object. It
> may change in the future, but at the moment I'm not aware of any
> use-case where these things can be different for different programs in
> the same object.
>
> I don't have strong preferences on the implementation side as long as it
> covers the use-case, e.g. the one in the patch set would work FWIW.
>
> > > 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.
>
> --
> Andrey Ignatov

Powered by blists - more mailing lists