Message-ID: <20200908174449.GA34763@rdna-mbp>
Date:   Tue, 8 Sep 2020 10:44:49 -0700
From:   Andrey Ignatov <rdna@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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 <zhuyifei@...gle.com>,
        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

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).

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