Re: [RFC PATCH bpf-next v2 00/11] mm, bpf: Add BPF into /proc/meminfo

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

On Tue, Jan 17, 2023 at 10:49 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > I just don't want to add many if-elses or switch-cases into
> > > bpf_map_memory_footprint(), because I think it is a little ugly.
> > > Introducing a new map ops could make it more clear.  For example,
> > > static unsigned long bpf_map_memory_footprint(const struct bpf_map *map)
> > > {
> > >     unsigned long size;
> > >
> > >     if (map->ops->map_mem_footprint)
> > >         return map->ops->map_mem_footprint(map);
> > >
> > >     size = round_up(map->key_size + bpf_map_value_size(map), 8);
> > >     return round_up(map->max_entries * size, PAGE_SIZE);
> > > }
> >
> > It is also ugly, because bpf_map_value_size() already has if-stmt.
> > I prefer to keep all estimates in one place.
> > There is no need to be 100% accurate.
>
> Per my investigation, it can be almost accurate with little effort.
> Take the htab for example,
> static unsigned long htab_mem_footprint(const struct bpf_map *map)
> {
>     struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>     unsigned long size = 0;
>
>     if (!htab_is_prealloc(htab)) {
>         size += htab_elements_size(htab);
>     }
>     size += kvsize(htab->elems);
>     size += percpu_size(htab->extra_elems);
>     size += kvsize(htab->buckets);
>     size += bpf_mem_alloc_size(&htab->pcpu_ma);
>     size += bpf_mem_alloc_size(&htab->ma);
>     if (htab->use_percpu_counter)
>         size += percpu_size(htab->pcount.counters);
>     size += percpu_size(htab->map_locked[i]) * HASHTAB_MAP_LOCK_COUNT;
>     size += kvsize(htab);
>     return size;
> }

Please don't.
Above doesn't look maintainable.
Look at kvsize(htab). Do you really care about hundred bytes?
Just accept that there will be a small constant difference
between what show_fdinfo reports and the real memory.
You cannot make it 100%.
There is kfence that will allocate 4k though you asked kmalloc(8).

> We just need to get the real memory size from the pointer instead of
> calculating the size again.
> For non-preallocated htab, it is a little trouble to get the element
> size (not the unit_size), but it won't be a big deal.

You'd have to convince mm folks that kvsize() is worth doing.
I don't think it will be easy.

> > With a callback devs will start thinking that this is somehow
> > a requirement to report precise memory.
> >
> > > > > > bpf side tracks all of its allocation. There is no need to do that
> > > > > > in generic mm side.
> > > > > > Exposing an aggregated single number if /proc/meminfo also looks wrong.
> > > > >
> > > > > Do you mean that we shouldn't expose it in /proc/meminfo ?
> > > >
> > > > We should not because it helps one particular use case only.
> > > > Somebody else might want map mem info per container,
> > > > then somebody would need it per user, etc.
> > >
> > > It seems we should show memcg info and user info in bpftool map show.
> >
> > Show memcg info? What do you have in mind?
> >
>
> Each bpf map is charged to a memcg. If we know a bpf map belongs to
> which memcg, we can know the map mem info per container.
> Currently we can get the memcg info from the process which loads it,
> but it can't apply to pinned-bpf-map.
> So it would be better if we can show it in bpftool-map-show.

That sounds useful.
Have you looked at bpf iterators and how bpftool is using
them to figure out which process loaded bpf prog and created particular map?