On Wed, Oct 23, 2024 at 5:54 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Tue, Oct 22, 2024 at 09:39:07PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > @@ -5146,11 +5147,43 @@ bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map *map)
> > if (err) {
> > err = -errno;
> > cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> > - pr_warn("Error freezing map(%s) as read-only: %s\n",
> > - map->name, cp);
> > + pr_warn("map '%s': failed to freeze as read-only: %s\n",
> > + bpf_map__name(map), cp);
> > return err;
> > }
> > }
> > +
> > + /* Remap anonymous mmap()-ed "map initialization image" as
> > + * a BPF map-backed mmap()-ed memory, but preserving the same
> > + * memory address. This will cause kernel to change process'
> > + * page table to point to a different piece of kernel memory,
> > + * but from userspace point of view memory address (and its
> > + * contents, being identical at this point) will stay the
> > + * same. This mapping will be released by bpf_object__close()
> > + * as per normal clean up procedure.
> > + */
> > + mmap_sz = bpf_map_mmap_sz(map);
> > + if (map->def.map_flags & BPF_F_MMAPABLE) {
> > + void *mmaped;
> > + int prot;
> > +
> > + if (map->def.map_flags & BPF_F_RDONLY_PROG)
> > + prot = PROT_READ;
> > + else
> > + prot = PROT_READ | PROT_WRITE;
> > + mmaped = mmap(map->mmaped, mmap_sz, prot, MAP_SHARED | MAP_FIXED, map->fd, 0);
> > + if (mmaped == MAP_FAILED) {
> > + err = -errno;
> > + pr_warn("map '%s': failed to re-mmap() contents: %d\n",
> > + bpf_map__name(map), err);
> > + return err;
> > + }
> > + map->mmaped = mmaped;
> > + } else if (map->mmaped) {
> > + munmap(map->mmaped, mmap_sz);
> > + map->mmaped = NULL;
> > + }
>
> this caught my eye because we did not do that in bpf_object__load_skeleton,
> makes sense, but why do we mmap *!*BPF_F_MMAPABLE maps in the first place?
The initial mmap(ANONYMOUS) is basically malloc(), but it works
uniformly for both BPF_F_MMAPABLE global data arrays, and non-mmapable
ones. Just a streamlining and thus simplification.
>
> jirka
>
> > +
> > return 0;
> > }
> >
> > @@ -5467,8 +5500,7 @@ bpf_object__create_maps(struct bpf_object *obj)
> > err = bpf_object__populate_internal_map(obj, map);
> > if (err < 0)
> > goto err_out;
> > - }
> > - if (map->def.type == BPF_MAP_TYPE_ARENA) {
> > + } else if (map->def.type == BPF_MAP_TYPE_ARENA) {
> > map->mmaped = mmap((void *)(long)map->map_extra,
> > bpf_map_mmap_sz(map), PROT_READ | PROT_WRITE,
> > map->map_extra ? MAP_SHARED | MAP_FIXED : MAP_SHARED,
> > @@ -13916,46 +13948,11 @@ int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
> > for (i = 0; i < s->map_cnt; i++) {
> > struct bpf_map_skeleton *map_skel = (void *)s->maps + i * s->map_skel_sz;
> > struct bpf_map *map = *map_skel->map;
> > - size_t mmap_sz = bpf_map_mmap_sz(map);
> > - int prot, map_fd = map->fd;
> > - void **mmaped = map_skel->mmaped;
> > -
> > - if (!mmaped)
> > - continue;
> > -
> > - if (!(map->def.map_flags & BPF_F_MMAPABLE)) {
> > - *mmaped = NULL;
> > - continue;
> > - }
> >
> > - if (map->def.type == BPF_MAP_TYPE_ARENA) {
> > - *mmaped = map->mmaped;
> > + if (!map_skel->mmaped)
> > continue;
> > - }
> > -
> > - if (map->def.map_flags & BPF_F_RDONLY_PROG)
> > - prot = PROT_READ;
> > - else
> > - prot = PROT_READ | PROT_WRITE;
> >
> > - /* Remap anonymous mmap()-ed "map initialization image" as
> > - * a BPF map-backed mmap()-ed memory, but preserving the same
> > - * memory address. This will cause kernel to change process'
> > - * page table to point to a different piece of kernel memory,
> > - * but from userspace point of view memory address (and its
> > - * contents, being identical at this point) will stay the
> > - * same. This mapping will be released by bpf_object__close()
> > - * as per normal clean up procedure, so we don't need to worry
> > - * about it from skeleton's clean up perspective.
> > - */
> > - *mmaped = mmap(map->mmaped, mmap_sz, prot, MAP_SHARED | MAP_FIXED, map_fd, 0);
> > - if (*mmaped == MAP_FAILED) {
> > - err = -errno;
> > - *mmaped = NULL;
> > - pr_warn("failed to re-mmap() map '%s': %d\n",
> > - bpf_map__name(map), err);
> > - return libbpf_err(err);
> > - }
> > + *map_skel->mmaped = map->mmaped;
> > }
> >
> > return 0;
> > --
> > 2.43.5
> >
> >
