Re: [RFC v2 PATCH bpf-next 0/4] bpf: add percpu stats for bpf_map

Daniel Borkmann
2023-06-26T09:18:03Z

On 6/24/23 2:17 AM, Alexei Starovoitov wrote:
> On Fri, Jun 23, 2023 at 2:53 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 6/22/23 11:53 AM, Anton Protopopov wrote:
>>> This series adds a mechanism for maps to populate per-cpu counters of elements
>>> on insertions/deletions. The sum of these counters can be accessed by a new
>>> kfunc from a map iterator program.
>>>
>>> The following patches are present in the series:
>>>
>>> * Patch 1 adds a generic per-cpu counter to struct bpf_map
>>> * Patch 2 utilizes this mechanism for hash-based maps
>>> * Patch 3 extends the preloaded map iterator to dump the sum
>>> * Patch 4 adds a self-test for the change
>>>
>>> The reason for adding this functionality in our case (Cilium) is to get
>>> signals about how full some heavy-used maps are and what the actual dynamic
>>> profile of map capacity is. In the case of LRU maps this is impossible to get
>>> this information anyhow else. See also [1].
>>>
>>> This is a v2 for the https://lore.kernel.org/bpf/20230531110511.64612-1-aspsk@isovalent.com/T/#t
>>> It was rewritten according to previous comments. I've turned this series into
>>> an RFC for two reasons:
>>>
>>> 1) This patch only works on systems where this_cpu_{inc,dec} is atomic for s64.
>>> For systems which might write s64 non-atomically this would be required to use
>>> some locking mechanism to prevent readers from reading trash via the
>>> bpf_map_sum_elements_counter() kfunc (see patch 1)
>>>
>>> 2) In comparison with the v1, we're adding extra instructions per map operation
>>> (for preallocated maps, as well as for non-preallocated maps). The only
>>> functionality we're interested at the moment is the number of elements present
>>> in a map, not a per-cpu statistics. This could be better achieved by using
>>> the v1 version, which only adds computations for preallocated maps.
>>>
>>> So, the question is: won't it be fine to do the changes in the following way:
>>>
>>> * extend the preallocated hash maps to populate percpu batch counters as in v1
>>> * add a kfunc as in v2 to get the current sum
>>>
>>> This works as
>>>
>>> * nobody at the moment actually requires the per-cpu statistcs
>>> * this implementation can be transparently turned into per-cpu statistics, if
>>> such a need occurs on practice (the only thing to change would be to
>>> re-implement the kfunc and, maybe, add more kfuncs to get per-cpu stats)
>>> * the "v1 way" is the least intrusive: it only affects preallocated maps, as
>>> other maps already provide the required functionality
>>>
>>> [1] https://lpc.events/event/16/contributions/1368/
>>>
>>> v1 -> v2:
>>> - make the counters generic part of struct bpf_map
>>> - don't use map_info and /proc/self/fdinfo in favor of a kfunc
>>
>> Tbh, I did like v1 approach a bit better. We are trying to bend over backwards just
>> so that we don't add things to uapi, but in the end we are also adding it to the
>> maps.debug, etc (yes, it has .debug in the name and all) ...
>
> I think we should keep bending even more backwards to avoid uapi burden.
> All new features should be non-uapi no matter how many people
> say "I'll definitely use it".
>
>> or as an extensible bpf_map_info stats extension in case there is some agreement?
>
> I'd rather not.
> bpf_map_info returns what user space sent to the kernel earlier.
> stats or anything that user space didn't explicitly give earlier
> is quite different.
> Same goes for bpf_prog_info.
> We only added verified_insns there that doesn't fit the above definition
> and it was a mistake. After almost 2 years it is still unused
> and cannot be removed.

Right, hence my question also on how others observe maps in production to see
if there are similar use cases and approaches as we have.

> veristat is parsing non-uapi verifier log.
> Tooling can live with non-uapi map stats just fine.

Sure, that's okay.

Thanks,
Daniel