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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.

> 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'?
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.
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.
                 */