Re: [v3 PATCH bpf-next 3/6] bpf: populate the per-cpu insertions/deletions counters for hashmaps

[Anton Protopopov <aspsk@isovalent.com>]

On Thu, Jul 06, 2023 at 10:01:26AM +0800, Hou Tao wrote:
> Hi,
> 
> On 7/4/2023 10:34 PM, Anton Protopopov wrote:
> > On Tue, Jul 04, 2023 at 09:56:36PM +0800, Hou Tao wrote:
> >> Hi,
> >>
> >> On 6/30/2023 4:25 PM, Anton Protopopov wrote:
> >>> Initialize and utilize the per-cpu insertions/deletions counters for hash-based
> >>> maps. Non-trivial changes only apply to the preallocated maps for which the
> >>> {inc,dec}_elem_count functions are not called, as there's no need in counting
> >>> elements to sustain proper map operations.
> >>>
> >>> To increase/decrease percpu counters for preallocated maps we add raw calls to
> >>> the bpf_map_{inc,dec}_elem_count functions so that the impact is minimal. For
> >>> dynamically allocated maps we add corresponding calls to the existing
> >>> {inc,dec}_elem_count functions.
> >>>
> >>> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> >>> ---
> >>>  kernel/bpf/hashtab.c | 23 ++++++++++++++++++++---
> >>>  1 file changed, 20 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> >>> index 56d3da7d0bc6..faaef4fd3df0 100644
> >>> --- a/kernel/bpf/hashtab.c
> >>> +++ b/kernel/bpf/hashtab.c
> >>> @@ -581,8 +581,14 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
> >>>  		}
> >>>  	}
> >>>  
> >>> +	err = bpf_map_init_elem_count(&htab->map);
> >>> +	if (err)
> >>> +		goto free_extra_elements;
> >> Considering the per-cpu counter is not always needed, is it a good idea
> >> to make the elem_count being optional by introducing a new map flag ?
> > Per-map-flag or a static key? For me it looked like just doing an unconditional
> > `inc` for a per-cpu variable is better vs. doing a check then `inc` or an
> > unconditional jump.
> 
> Sorry I didn't make it clear that I was worried about the allocated
> per-cpu memory. Previous I thought the per-cpu memory is limited, but
> after did some experiments I found it was almost the same as kmalloc()
> which could use all available memory to fulfill the allocation request.
> For a host with 72-cpus, the memory overhead for 10k hash map is about
> ~6MB. The overhead is tiny compared with the total available memory, but
> it is avoidable.

So, in my first patch I've only added new counters for preallocated maps. But
then the feedback was that we need a generic percpu inc/dec counters, so I
added them by default. For me a percpu s64 looks cheap enough for a hash map...