On Tue, Sep 8, 2020 at 10:45 AM Andrey Ignatov [off-list ref] wrote:

Andrii Nakryiko [off-list ref] [Fri, 2020-09-04 16:19 -0700]:

quoted

On Thu, Sep 3, 2020 at 6:29 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Sep 02, 2020 at 07:31:33PM -0700, Andrii Nakryiko wrote:

quoted

On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev [off-list ref] wrote:

quoted

From: YiFei Zhu <redacted>

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

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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