Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section

On Wed, Apr 14, 2021 at 04:48:25PM -0700, Andrii Nakryiko wrote:

On Wed, Apr 14, 2021 at 3:00 PM Alexei Starovoitov [off-list ref] wrote:

quoted

On 4/14/21 1:01 PM, Andrii Nakryiko wrote:

quoted

Add extra logic to handle map externs (only BTF-defined maps are supported for
linking). Re-use the map parsing logic used during bpf_object__open(). Map
externs are currently restricted to always and only specify map type, key
type and/or size, and value type and/or size. Nothing extra is allowed. If any
of those attributes are mismatched between extern and actual map definition,
linker will report an error.

I don't get the motivation for this.
It seems cumbersome to force users to do:
+extern struct {
+       __uint(type, BPF_MAP_TYPE_HASH);
+       __type(key, key_type);
+       __type(value, value_type);
+       /* no max_entries on extern map definitions */
+} map1 SEC(".maps");

The intent was to simulate what you'd have in a language with
generics. E.g., if you were declaring extern for a map in C++:

extern std::map<key_type, value_type> my_map;

right, because C++ will mangle types into names.
When llvm bpf backend will support C++ front-end it will do the mangling too.
I think BPF is ready for C++, but it's a separate discussion, of course.

quoted

but there is only one such full map definition.
Can all externs to be:
extern struct {} map1 SEC(".maps");

I can certainly modify logic to allow this. But for variables and
funcs we want to enforce type information, right? So I'm not sure why
you think it's bad for maps.

I'm not saying it's bad.
Traditional linker only deals with names, since we're in C domain, so far,
I figured it's an option, but more below.
C++ is good analogy too.

So if it's just a multi-file application and you don't care which file
declares that map, you can do a single __weak definition in a header
and forget about it.

But imagine a BPF library, maintained separately from some BPF
application that is using it. And imagine that for some reason that
BPF library wants/needs to "export" its map directly. In such case,
I'd imagine BPF library author to provide a header with pre-defined
correct extern definition of that map.

I'm mainly looking at patch 17 and thinking how that copy paste can be avoided.
In C and C++ world the user would do:
defs.h:
  struct S {
    ...
  };
  extern struct S s;
file.c:
  #include "defs.h"
  struct S s;
and it would work, but afaics it won't work for BPF C in patch 17.
If the user does:
defs.h:
  struct my_map {
          __uint(type, BPF_MAP_TYPE_HASH);
          __type(key, struct my_key);
          __type(value, struct my_value);
          __uint(max_entries, 16);
  };
  extern struct my_map map1 SEC(".maps");
file.c:
  #include "defs.h"
  struct my_map map1;  // do we need SEC here too? probably not?

It won't work for another_filer.c since max_entries are not allowed?
Why, btw?

So how the user suppose to do this? With __weak in .h ?
But if that's the only reasonable choice whe bother supporting extern in the linker?

I originally wanted to let users define which attributes matter and
enforce them (as I mention in the commit description), but that
requires some more work on merging BTF. Now that I'm done with all the
rest logic, I guess I can go and address that as well.

I think that would be overkill. It won't match neither C style nor C++.
Let's pick one.

So see above about __weak. As for the BPF library providers, that felt
unavoidable (and actually desirable), because that's what they would
do with extern func and extern vars anyways.

As far as supporting __weak for map defs, I think __weak in one file.c
should be weak for all attributes. Another_file.c should be able
to define the same map name without __weak and different types, value/type
sizes. Because why not? Sort-of C++ style of override.

so forcing to type+key+value is to make sure that currently all
externs (if there are many) are exactly the same. Because as soon as I
allow some to specify max_entries and some don't,

I don't get why max_entries is special.
They can be overridden in typical skeleton usage. After open and before load.
So max_entries is a default value in map init. Whether it's part of
extern or not why should that matter?

Maybe nothing, just there is no single right answer (except the
aspirational implementation I explained above). I'm open to
discussion, btw, not claiming my way is the best way.

I'm not suggesting that extern struct {} my_map; is the right answer either.
Mainly looking into how user code will look like and trying to
make it look the most similar to how C, C++ code traditionally looks.
BPF C is reduced and extended C at the same time.
BPF C++ will be similar. Certain features will be supported right away,
some others will take time.
I'm looking at BTF as a language independent concept.
Both C and C++ will rely on it.

To summarize if max_entries can be supported and ingored in extern
when the definition has a different value then it's probably good to enforce
that the rest of map fields are the same. Then my .h/.c example above will work.
In case of __weak probably all map fields can change.
It can be seen as a weak definition of the whole map. Not just weak of the variable.
It's a default for everything that can be overridden.
While non-weak can override max_entries only.

btw for signed progs I'm thinking to allow override of max_entries only,
since this attribute doesn't affect safety, correctness, behavior.
Meaning max_entries will and will not be part of a signature at the same time.
In other words it's necessary to support existing bcc/libbpf-tools.
If we go with 'allow max_entries in extern' that would match that behavior.