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

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

On Tue, Feb 13, 2024 at 3:11 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-02-08 at 20:06 -0800, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > LLVM automatically places __arena variables into ".arena.1" ELF section.
> > When libbpf sees such section it creates internal 'struct bpf_map' LIBBPF_MAP_ARENA
> > that is connected to actual BPF_MAP_TYPE_ARENA 'struct bpf_map'.
> > They share the same kernel's side bpf map and single map_fd.
> > Both are emitted into skeleton. Real arena with the name given by bpf program
> > in SEC(".maps") and another with "__arena_internal" name.
> > All global variables from ".arena.1" section are accessible from user space
> > via skel->arena->name_of_var.
> >
> > For bss/data/rodata the skeleton/libbpf perform the following sequence:
> > 1. addr = mmap(MAP_ANONYMOUS)
> > 2. user space optionally modifies global vars
> > 3. map_fd = bpf_create_map()
> > 4. bpf_update_map_elem(map_fd, addr) // to store values into the kernel
> > 5. mmap(addr, MAP_FIXED, map_fd)
> > after step 5 user spaces see the values it wrote at step 2 at the same addresses
> >
> > arena doesn't support update_map_elem. Hence skeleton/libbpf do:
> > 1. addr = mmap(MAP_ANONYMOUS)
> > 2. user space optionally modifies global vars
> > 3. map_fd = bpf_create_map(MAP_TYPE_ARENA)
> > 4. real_addr = mmap(map->map_extra, MAP_SHARED | MAP_FIXED, map_fd)
> > 5. memcpy(real_addr, addr) // this will fault-in and allocate pages
> > 6. munmap(addr)
> >
> > At the end look and feel of global data vs __arena global data is the same from bpf prog pov.
>
> [...]
>
> So, at first I thought that having two maps is a bit of a hack.
> However, after trying to make it work with only one map I don't really
> like that either :)

My first attempt was with single arena map, but it ended up with
hacks all over libbpf and bpftool to treat one map differently depending
on conditions.
Two maps simplified the code a lot.

> The patch looks good to me, have not spotted any logical issues.
>
> I have two questions if you don't mind:
>
> First is regarding initialization data.
> In bpf_object__init_internal_map() the amount of bpf_map_mmap_sz(map)
> bytes is mmaped and only data_sz bytes are copied,
> then bpf_map_mmap_sz(map) bytes are copied in bpf_object__create_maps().
> Is Linux/libc smart enough to skip action on pages which were mmaped but
> never touched?

kernel gives zeroed out pages to user space.
So it's ok to mmap a page, copy data_sz bytes into it
and later copy the full page from one addr to another.
No garbage copy.
In this case there will be data by llvm construction of ".arena.1"
It looks to me that even .bss-like __arena vars have zero-s in data
and non-zero data_sz.

>
> Second is regarding naming.
> Currently only one arena is supported, and generated skel has a single '->arena' field.
> Is there a plan to support multiple arenas at some point?
> If so, should '->arena' field use the same name as arena map declared in program?

I wanted to place all global arena vars into a default name "arena"
and let skeleton to use that name without thinking what name
bpf prog gave to the actual map.