Message-ID: <20190305022856.uynexxxywfixzvl7@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 4 Mar 2019 18:28:58 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <ast@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "joe@...d.net.nz" <joe@...d.net.nz>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "tgraf@...g.ch" <tgraf@...g.ch>, Andrii Nakryiko <andriin@...com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "lmb@...udflare.com" <lmb@...udflare.com>
Subject: static bpf vars. Was: [PATCH bpf-next v2 5/7] bpf, libbpf: support
 global data/bss/rodata sections

On Fri, Mar 01, 2019 at 09:06:35PM +0100, Daniel Borkmann wrote:
> 

Overall I think the patches and direction is great.
Thanks a lot for working on it.
More thoughts below:

> By the way, from LLVM side, do you think it makes sense for local vars
> where you encode the offset into insn->imm to already encode it into
> (insn+1)->imm of the ldimm64, so that loaders can just pass this offset
> through instead of fixing it up like I did? I'm fine either way though,
> just thought might be worth pointing out while we're at it. :)

1.
typical ldimm64 carries 64-bit value. upper 32-bits are in insn[1]->imm.
I think it's better to stick to this meaning of imm for encoding
the section offset and keep it in insn[0]->imm.
Especially since from disassembler pov it looks like normal ldimm64 with relocation.
Later libbpf does PSEUDO trick and can move [0]->imm to [1]->imm.
Or it can use [1]->imm to store FD to pass to kernel?
Another alternative is to use 16-bit of 'off' to encode the offset,
but the limit of 32kbyte of static variables is probably too small.

2.
I think it's better to only allow 'static int foo' in libbpf for now.
In C 'global' variables suppose to be shareable across multiple elf files.
libbpf doesn't have support for it yet. We need to start discussing this multi elf
support which will lead into discussion about bpf libraries.
I think we need a bit of freedom to decide the meaning of 'global' modifier.
So for now I would support only 'static int foo' in libbpf.
llvm supports both and that's ok, since that's a standard elf markings.

Also were these patches tested with 'static struct {int a; int b;} foo;' and
'static char foo[]="my string";' ?
I suspect it should 'just work' with proposed design, but would be good to have a test.

3.
let's come up with concrete plan for BTF for these static variables.
With normal maps the BTF was retrofitted via BPF_ANNOTATE_KV_PAIR macro
and no one is proud of this interface choice. We had 'struct bpf_map_def'
and the macro was the least evil.
For static variables let's figure out how to integrate BTF into it cleanly.
LLVM already knows about the types. It needs another kind to describe
the variable names and offsets. I think what we did with BTF_KIND_FUNC
should be applicable here. Last time we discussed that we will add BTF_KIND_VAR
for this purpose. Shouldn't be too much work to hack it into llvm now?
I would even go a step further and make BTF to be mandatory for static vars.
Introspection of BPF programs will go long way.

4.
if we make BTF mandatory then the issue of map names will be solved too.
The <obj>.bss/data/rodata might not fit into map_name[16],
but with BTF we can give it full name.
"<obj>.data" looks great to me especially from introspection angle.

5.
I think it makes sense to keep .bss vs .data vs .rodata separate.
I can imagine a program using large zero-inited static array.
It's important to save space in elf file, save time to do map create
and map populate. With combined .data and .bss it would be much harder.

6.
as the next step would be good to add two new syscall command to read/write
individual static variables. Something like:
bpf_map_read/write_bytes(map, key, offset_in_value, buf, size_of_buf)
would allow user space to tweak certain variables.
With mandatory BTF we can have an api to read/write variable by name
(and not only by offset).
Such bpf_map_read_bytes() would be useful for regular hash maps too.
It's convenient to be able to update few bytes in the hash value instead
of overwriting the whole thing.

7.
since these 3 maps are created as normal arrays from kernel pov
the 'bpftool map show' should not exclude them from the output.
Similarly libbpf api bpf_object__for_each_map() and bpf_map__next()
should probably include them too. There can be bpf_map__is_pseudo() flag
to tell them apart, but since it's a normal map in the kernel
other libbpf accessors bpf_map__name(), bpf_map__pin() should work too.

Powered by blists - more mailing lists