Re: [PATCH bpf-next 2/5] bpf: Implement cgroup storage available to non-cgroup-attached bpf progs

[Yonghong Song <yhs@meta.com>]

On 10/18/22 11:11 AM, Yosry Ahmed wrote:
> On Tue, Oct 18, 2022 at 11:08 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 10/18/22 10:17 AM, Alexei Starovoitov wrote:
>>> On Tue, Oct 18, 2022 at 10:08 AM <sdf@google.com> wrote:
>>>>>>
>>>>>> '#define BPF_MAP_TYPE_CGROUP_STORAGE BPF_MAP_TYPE_CGRP_LOCAL_STORAGE /*
>>>>>> depreciated by BPF_MAP_TYPE_CGRP_STORAGE */' in the uapi.
>>>>>>
>>>>>> The new cgroup storage uses a shorter name "cgrp", like
>>>>>> BPF_MAP_TYPE_CGRP_STORAGE and bpf_cgrp_storage_get()?
>>>>
>>>>> This might work and the naming convention will be similar to
>>>>> existing sk/inode/task storage.
>>>>
>>>> +1, CGRP_STORAGE sounds good!
>>>
>>> +1 from me as well.
>>>
>>> Something like this ?
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 17f61338f8f8..13dcb2418847 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -922,7 +922,8 @@ enum bpf_map_type {
>>>           BPF_MAP_TYPE_CPUMAP,
>>>           BPF_MAP_TYPE_XSKMAP,
>>>           BPF_MAP_TYPE_SOCKHASH,
>>> -       BPF_MAP_TYPE_CGROUP_STORAGE,
>>> +       BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED,
>>> +       BPF_MAP_TYPE_CGROUP_STORAGE = BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED,
>>
>> +1
>>
>>>           BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
>>>           BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
>>>           BPF_MAP_TYPE_QUEUE,
>>> @@ -935,6 +936,7 @@ enum bpf_map_type {
>>>           BPF_MAP_TYPE_TASK_STORAGE,
>>>           BPF_MAP_TYPE_BLOOM_FILTER,
>>>           BPF_MAP_TYPE_USER_RINGBUF,
>>> +       BPF_MAP_TYPE_CGRP_STORAGE,
>>>    };

Sounds good to me. Will do this in the next revision.

>>>
>>> What are we going to do with BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE ?
>>> Probably should come up with a replacement as well?
>>
>> Yeah, need to come up with a percpu answer for it.  The percpu usage has never
>> come up on the sk storage and also the later task/inode storage.  or the user is
>> just getting by with an array like map's value.
>>
>> May be the bpf prog can call bpf_mem_alloc() to alloc the percpu memory in the
>> future and then store it as the kptr in the BPF_MAP_TYPE_CGRP_STORAGE?
> 
> A percpu cgroup storage would be very beneficial for cgroup statistics
> collection, things like the selftest in
> tools/testing/selftests/bpf/progs/cgroup_hierarchical_stats.c
> currently uses a percpu hashmap indexed by cgroup id, so using a
> percpu cgroup storage instead would be a nice upgrade.

Indeed, agree. For cgroup storage, we could have a per-cpu version
for the new mechanism so it can replace the old one as well.
Will look into this after non per-cpu version is done.