Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section
On 4/15/21 1:35 PM, Andrii Nakryiko wrote:
How about we start in the most restrictive way first. Each extern
would need to specify all the attributes that should match the map
definition. That includes max_entries. That way the typedef struct {
... } my_map_t re-use will work right out of the box. Later, if we see
this is not sufficient, we can start relaxing the rules.
+1
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.
Ok, unless I misunderstood, allowing and checking all map attributes
as a starting point should work, right?
yes. thanks!
Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section
On Wed, Apr 14, 2021 at 7:01 PM Alexei Starovoitov
wrote:
>
> On Wed, Apr 14, 2021 at 04:48:25PM -0700, Andrii Nakryiko wrote:
> > On Wed, Apr 14, 2021 at 3:00 PM Alexei Starovoitov 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 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.
Yes, you are right, there is no clean way to avoid defining extern and
full map definition. Which is the case for functions and variables,
except those type signatures tend to be shorter. E.g., if you had
void my_func(int arg) { ... }
you'd still have to duplicate it as at least:
extern void my_func(int);
I don't think you can use typedef for this either.
> 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?
yeah, we do, all map "variables" are designated with .maps section,
otherwise they'll be treated as just a normal global variable (there
is no way to distinguish two, generally speaking).
>
> It won't work for another_filer.c since max_entries are not allowed?
> Why, btw?
So the idea was that for consumers of extern map definition map type
and key/value info was the only thing they should care about and
linker should enforce (at least that's how I thought about this and
what I think the typical use case would be). E.g., caring about exact
max_entries, or numa_node, or pinning, or map_flags, etc, shouldn't be
the concern of the consumer of the map.
>
> 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 I still think we might want to implement it down the road, but
let's stick to something simpler for now. See below.
>
> > 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
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 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 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
Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section
On Wed, Apr 14, 2021 at 3:00 PM Alexei Starovoitov 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 my_map;
You'd want a linker to make sure that actual my_map definition to
conform to your expectations, no?
>
> > The original intent was to allow for extern to specify attributes that
> > matters
> > (to user) to enforce. E.g., if you specify just key information and omit
> > value, then any value fits. Similarly, it should have been possible to
> > enforce
> > map_flags, pinning, and any other possible map attribute. Unfortunately,
> > that
> > means that multiple externs can be only partially overlapping with each
> > other,
> > which means linker would need to combine their type definitions to end up
> > with
> > the most restrictive and fullest map definition.
>
> 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.
>
> They can be in multiple .o files, but one true global map def
> should have all the fields and will take the precedence during
> the linking.
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.
It's the same situation as with extern functions. You are either
copy/pasting exact function signature or providing it through some
common header. BPF map definition is just slightly more verbose.
>
> The map type, key size, value size, max entries are all irrelevant
> during compilation. They're relevant during loading, but libbpf is
> not going to load every .o individually. So "extern map" can
> have any fields it wouldn't change the end result after linking.
> May be enforce that 'extern struct {} map' doesn't have
> any fields defined instead?
It's easy for me to do that as well, it's just a question of what
behavior makes more sense and what are we trying to achieve. Of course
during the compilation itself it doesn't matter that's the type of map
is or what key/value type/size is. But from the programmer's point of
view, when I do lookup/update, I'd like to know that my map
corresponds to my understanding. So if I assume 4-byte key, and
16-byte value and allocate stack variables according to that
understanding, yet something changes about BPF map definition, I'd
rather notice that during linking, than maybe notice during BPF
verification. So that was the only motivation: catch mismatch earlier.
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. So that would
support cases from:
extern struct {} my_map SEC(".maps");
to
extern struct {
__uint(type, BPF_MAP_TYPE_ARRAY);
__type(key, int);
__type(value, struct value_type);
__uint(map_flags, BPF_F_MMAPABLE); /* because I care for whatever reason */
__uint(pinning, LIBBPF_PIN_BY_NAME); /* because I can */
} my_peculiar_map SEC(".maps");
But basically, if we allow only `extern struct {} my_map
SEC(".maps");`, why do I even bother with BTF in that case?
> It seems asking users to copy-paste map defs in one file and in all
> of extern is just extra hassle.
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. And that's what we do
with C code today, except linker is oblivious to types (because no BTF
in user-space C world).
> The users wouldn't want to copy-paste them for pr
Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section
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 original intent was to allow for extern to specify attributes that matters
(to user) to enforce. E.g., if you specify just key information and omit
value, then any value fits. Similarly, it should have been possible to enforce
map_flags, pinning, and any other possible map attribute. Unfortunately, that
means that multiple externs can be only partially overlapping with each other,
which means linker would need to combine their type definitions to end up with
the most restrictive and fullest map definition.
but there is only one such full map definition.
Can all externs to be:
extern struct {} map1 SEC(".maps");
They can be in multiple .o files, but one true global map def
should have all the fields and will take the precedence during
the linking.
The map type, key size, value size, max entries are all irrelevant
during compilation. They're relevant during loading, but libbpf is
not going to load every .o individually. So "extern map" can
have any fields it wouldn't change the end result after linking.
May be enforce that 'extern struct {} map' doesn't have
any fields defined instead?
It seems asking users to copy-paste map defs in one file and in all
of extern is just extra hassle.
The users wouldn't want to copy-paste them for production code,
but will put map def into .h and include in multiple .c,
but adding "extern " in many .c-s and not
adding that "extern " is the main .c is another macro hassle.
Actually forcing "no max_entries in extern" is killing this idea.
So it's mandatory copy-paste or even more macro magic with partial
defs of maps?
What am I missing?
