From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Alexei Starovoitov <ast@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Stanislav Fomichev <sdf@fomichev.me>,
Andrii Nakryiko <andriin@fb.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Kernel Team <Kernel-team@fb.com>, Yonghong Song <yhs@fb.com>
Subject: Re: explicit maps. Was: [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF
Date: Mon, 10 Jun 2019 14:15:28 -0700 [thread overview]
Message-ID: <20190610141528.38c71524@cakuba.netronome.com> (raw)
In-Reply-To: <b9798871-3b0e-66ce-903d-c9a587651abc@fb.com>
On Mon, 10 Jun 2019 01:17:13 +0000, Alexei Starovoitov wrote:
> On 6/6/19 6:02 PM, Jakub Kicinski wrote:
> > On Fri, 7 Jun 2019 00:27:52 +0000, Alexei Starovoitov wrote:
> >> the solution we're discussing should solve BPF_ANNOTATE_KV_PAIR too.
> >> That hack must go.
> >
> > I see.
> >
> >> If I understood your objections to Andrii's format is that
> >> you don't like pointer part of key/value while Andrii explained
> >> why we picked the pointer, right?
> >>
> >> So how about:
> >>
> >> struct {
> >> int type;
> >> int max_entries;
> >> struct {
> >> __u32 key;
> >> struct my_value value;
> >> } types[];
> >> } ...
> >
> > My objection is that k/v fields are never initialized, so they're
> > "metafields", mixed with real fields which hold parameters - like
> > type, max_entries etc.
>
> I don't share this meta fields vs real fields distinction.
> All of the fields are meta.
> Kernel implementation of the map doesn't need to hold type and
> max_entries as actual configuration fields.
> The map definition in c++ would have looked like:
> bpf::hash_map<int, struct my_value, 1000, NO_PREALLOC> foo;
> bpf::array_map<struct my_value, 2000> bar;
>
> Sometime key is not necessary. Sometimes flags have to be zero.
> bpf syscall api is a superset of all fiels for all maps.
> All of them are configuration and meta fields at the same time.
> In c++ example there is really no difference between
> 'struct my_value' and '1000' attributes.
>
> I'm pretty sure bpf will have C++ front-end in the future,
> but until then we have to deal with C and, I think, the map
> definition should be the most natural C syntax.
> In that sense what you're proposing with extern:
> > extern struct my_key my_key;
> > extern int type_int;
> >
> > struct map_def {
> > int type;
> > int max_entries;
> > void *btf_key_ref;
> > void *btf_val_ref;
> > } = {
> > ...
> > .btf_key_ref = &my_key,
> > .btf_val_ref = &type_int,
> > };
>
> is worse than
>
> struct map_def {
> int type;
> int max_entries;
> int btf_key;
> struct my_key btf_value;
> };
>
> imo explicit key and value would be ideal,
> but they take too much space. Hence pointers
> or zero sized array:
> struct {
> int type;
> int max_entries;
> struct {
> __u32 key;
> struct my_value value;
> } types[];
> };
It is a C syntax problem, I do agree with you that it works well for
templates. The map_def structure holds parameters, and we can't take
a type as a value in C. Hence the types[] in your proposal - you could
as well call them ghost_fields[] :)
> I think we should also consider explicit map creation.
>
> Something like:
>
> struct my_map {
> __u32 key;
> struct my_value value;
> } *my_hash_map, *my_pinned_hash_map;
>
> struct {
> __u64 key;
> struct my_map *value;
> } *my_hash_of_maps;
>
> struct {
> struct my_map *value;
> } *my_array_of_maps;
>
> __init void create_my_maps(void)
> {
> bpf_create_hash_map(&my_hash_map, 1000/*max_entries*/);
> bpf_obj_get(&my_pinned_hash_map, "/sys/fs/bpf/my_map");
> bpf_create_hash_of_maps(&my_hash_of_maps, 1000/*max_entries*/);
> bpf_create_array_of_maps(&my_array_of_maps, 20);
> }
>
> SEC("cgroup/skb")
> int bpf_prog(struct __sk_buff *skb)
> {
> struct my_value *val;
> __u32 key;
> __u64 key64;
> struct my_map *map;
>
> val = bpf_map_lookup(my_hash_map, &key);
> map = bpf_map_lookup(my_hash_of_maps, &key64);
> }
>
> '__init' section will be compiled by llvm into bpf instructions
> that will be executed in users space by libbpf.
> The __init prog has to succeed otherwise prog load fails.
>
> May be all map pointers should be in a special section to avoid
> putting them into datasec, but libbpf should be able to figure that
> out without requiring user to specify the .map section.
> The rest of global vars would go into special datasec map.
>
> No llvm changes necessary and BTF is available for keys and values.
>
> libbpf can start with simple __init and eventually grow into
> complex init procedure where maps are initialized,
> prog_array is populated, etc.
>
> Thoughts?
I like it! :)
next prev parent reply other threads:[~2019-06-10 21:15 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-31 20:21 [RFC PATCH bpf-next 0/8] BTF-defined BPF map definitions Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 1/8] libbpf: add common min/max macro to libbpf_internal.h Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 2/8] libbpf: extract BTF loading and simplify ELF parsing logic Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 3/8] libbpf: refactor map initialization Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 4/8] libbpf: identify maps by section index in addition to offset Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 5/8] libbpf: split initialization and loading of BTF Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF Andrii Nakryiko
2019-05-31 21:28 ` Stanislav Fomichev
2019-05-31 22:58 ` Andrii Nakryiko
2019-06-03 0:33 ` Jakub Kicinski
2019-06-03 21:54 ` Andrii Nakryiko
2019-06-03 23:34 ` Jakub Kicinski
2019-06-03 16:32 ` Stanislav Fomichev
2019-06-03 22:03 ` Andrii Nakryiko
2019-06-04 1:02 ` Stanislav Fomichev
2019-06-04 1:07 ` Alexei Starovoitov
2019-06-04 4:29 ` Stanislav Fomichev
2019-06-04 13:45 ` Stanislav Fomichev
2019-06-04 17:31 ` Andrii Nakryiko
2019-06-04 21:07 ` Stanislav Fomichev
2019-06-04 21:22 ` Andrii Nakryiko
2019-06-06 21:09 ` Daniel Borkmann
2019-06-06 23:02 ` Andrii Nakryiko
2019-06-06 23:27 ` Alexei Starovoitov
2019-06-07 0:10 ` Jakub Kicinski
2019-06-07 0:27 ` Alexei Starovoitov
2019-06-07 1:02 ` Jakub Kicinski
2019-06-10 1:17 ` explicit maps. Was: " Alexei Starovoitov
2019-06-10 21:15 ` Jakub Kicinski [this message]
2019-06-10 23:48 ` Andrii Nakryiko
2019-06-03 22:34 ` Andrii Nakryiko
2019-06-06 16:42 ` Lorenz Bauer
2019-06-06 22:34 ` Andrii Nakryiko
2019-06-17 9:07 ` Lorenz Bauer
2019-06-17 20:59 ` Andrii Nakryiko
2019-06-20 9:27 ` Lorenz Bauer
2019-06-21 4:05 ` Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 7/8] selftests/bpf: add test for BTF-defined maps Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 8/8] selftests/bpf: switch tests to BTF-defined map definitions Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190610141528.38c71524@cakuba.netronome.com \
--to=jakub.kicinski@netronome.com \
--cc=Kernel-team@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=netdev@vger.kernel.org \
--cc=sdf@fomichev.me \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).