Re: [PATCH v2 bpf-next 14/20] libbpf: Recognize __arena global varaibles.

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Feb 13, 2024 at 5:24 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 13, 2024 at 4:09 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Feb 13, 2024 at 3:37 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Tue, 2024-02-13 at 15:17 -0800, Andrii Nakryiko wrote:
> > >
> > > [...]
> > >
> > > > > So, at first I thought that having two maps is a bit of a hack.
> > > >
> > > > yep, that was my instinct as well
> > > >
> > > > > However, after trying to make it work with only one map I don't really
> > > > > like that either :)
> > > >
> > > > Can you elaborate? see my reply to Alexei, I wonder how did you think
> > > > about doing this?
> > >
> > > Relocations in the ELF file are against a new section: ".arena.1".
> > > This works nicely with logic in bpf_program__record_reloc().
> > > If single map is used, we effectively need to track two indexes for
> > > the map section:
> > > - one used for relocations against map variables themselves
> > >   (named "generic map reference relocation" in the function code);
> > > - one used for relocations against ".arena.1"
> > >   (named "global data map relocation" in the function code).
> > >
> > > This spooked me off:
> > > - either bpf_object__init_internal_map() would have a specialized
> > >   branch for arenas, as with current approach;
> > > - or bpf_program__record_reloc() would have a specialized branch for arenas,
> > >   as with one map approach.
> >
> > Yes, relocations would know about .arena.1, but it's a pretty simple
> > check in a few places. We basically have arena *definition* sec_idx
> > (corresponding to SEC(".maps")) and arena *data* sec_idx. The latter
> > is what is recorded for global variables in .arena.1. We can remember
> > this arena data sec_idx in struct bpf_object once during ELF
> > processing, and then just special case it internally in a few places.
>
> That was my first attempt and bpf_program__record_reloc()
> became a mess.
> Currently it does relo search either in internal maps
> or in obj->efile.btf_maps_shndx.
> Doing double search wasn't nice.
> And further, such dual meaning of 'struct bpf_map' object messes
> assumptions of bpf_object__shndx_is_maps, bpf_object__shndx_is_data
> and the way libbpf treats map->libbpf_type everywhere.
>
> bpf_map__is_internal() cannot really say true or false
> for such dual use map.
> Then skeleton gen gets ugly.
> Needs more public libbpf APIs to use in bpftool gen.
> Just a mess.

It might be easier for me to try implement it the way I see it than
discuss it over emails. I'll give it a try today-tomorrow and get back
to you.

>
> > The "fake" bpf_map for __arena_internal is user-visible and requires
> > autocreate=false tricks, etc. I feel like it's a worse tradeoff from a
> > user API perspective than a few extra ARENA-specific internal checks
> > (which we already have a few anyways, ARENA is not completely
> > transparent internally anyways).
>
> what do you mean 'user visible'?

That __arena_internal (representing .area.1 data section) actually is
separate from actual ARENA map (represented by variable in .maps
section). And both have separate `struct bpf_map`, which you can look
up by name or through iterating all maps of bpf_object. And that you
can call getters/setters on __arena_internal, even though the only
thing that actually makes sense there is bpf_map__initial_value(),
which would just as much make sense on ARENA map itself.

> I can add a filter to avoid generating a pointer for it in a skeleton.
> Then it won't be any more visible than other bss/data fake maps.

bss/data are not fake maps, they have corresponding BPF map (ARRAY) in
the kernel. Which is different from __arena_internal. And even if we
hide it from skeleton, it's still there in bpf_object, as I mentioned
above.

Let me try implementing what I have in mind and see how bad it is.

> The 2nd fake arena returns true out of bpf_map__is_internal.
>
> The key comment in the patch:
>                 /* bpf_object will contain two arena maps:
>                  * LIBBPF_MAP_ARENA & BPF_MAP_TYPE_ARENA
>                  * and
>                  * LIBBPF_MAP_UNSPEC & BPF_MAP_TYPE_ARENA.
>                  * The former map->arena will point to latter.
>                  */

Yes, and I'd like to not have two arena maps because they are logically one.