Message-ID: <20210415020138.2dbcflpxq2zwu6b2@ast-mbp>
Date:   Wed, 14 Apr 2021 19:01:38 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...com>,
        Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: 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 <ast@...com> wrote:
> >
> > On 4/14/21 1:01 PM, Andrii Nakryiko wrote:
> > > 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.

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

Powered by blists - more mailing lists