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

[Yonghong Song <yhs@meta.com>]

On 10/17/22 5:52 PM, Martin KaFai Lau wrote:
> On 10/17/22 3:16 PM, sdf@google.com wrote:
>> On 10/17, Martin KaFai Lau wrote:
>>> On 10/17/22 12:11 PM, Yosry Ahmed wrote:
>>> > On Mon, Oct 17, 2022 at 12:07 PM Stanislav Fomichev 
>>> <sdf@google.com> wrote:
>>> > >
>>> > > On Mon, Oct 17, 2022 at 11:47 AM Yosry Ahmed 
>>> <yosryahmed@google.com> wrote:
>>> > > >
>>> > > > On Mon, Oct 17, 2022 at 11:43 AM Stanislav Fomichev 
>>> <sdf@google.com> wrote:
>>> > > > >
>>> > > > > On Mon, Oct 17, 2022 at 11:26 AM Yosry Ahmed 
>>> <yosryahmed@google.com> wrote:
>>> > > > > >
>>> > > > > > On Mon, Oct 17, 2022 at 11:02 AM <sdf@google.com> wrote:
>>> > > > > > >
>>> > > > > > > On 10/13, Yonghong Song wrote:
>>> > > > > > > > Similar to sk/inode/task storage, implement similar 
>>> cgroup local storage.
>>> > > > > > >
>>> > > > > > > > There already exists a local storage implementation for 
>>> cgroup-attached
>>> > > > > > > > bpf programs.  See map type BPF_MAP_TYPE_CGROUP_STORAGE 
>>> and helper
>>> > > > > > > > bpf_get_local_storage(). But there are use cases such 
>>> that non-cgroup
>>> > > > > > > > attached bpf progs wants to access cgroup local storage 
>>> data. For example,
>>> > > > > > > > tc egress prog has access to sk and cgroup. It is 
>>> possible to use
>>> > > > > > > > sk local storage to emulate cgroup local storage by 
>>> storing data in
>>> > > > > > > > socket.
>>> > > > > > > > But this is a waste as it could be lots of sockets 
>>> belonging to a
>>> > > > > > > > particular
>>> > > > > > > > cgroup. Alternatively, a separate map can be created 
>>> with cgroup id as
>>> > > > > > > > the key.
>>> > > > > > > > But this will introduce additional overhead to 
>>> manipulate the new map.
>>> > > > > > > > A cgroup local storage, similar to existing 
>>> sk/inode/task storage,
>>> > > > > > > > should help for this use case.
>>> > > > > > >
>>> > > > > > > > The life-cycle of storage is managed with the 
>>> life-cycle of the
>>> > > > > > > > cgroup struct.  i.e. the storage is destroyed along 
>>> with the owning cgroup
>>> > > > > > > > with a callback to the bpf_cgroup_storage_free when 
>>> cgroup itself
>>> > > > > > > > is deleted.
>>> > > > > > >
>>> > > > > > > > The userspace map operations can be done by using a 
>>> cgroup fd as a key
>>> > > > > > > > passed to the lookup, update and delete operations.
>>> > > > > > >
>>> > > > > > >
>>> > > > > > > [..]
>>> > > > > > >
>>> > > > > > > > Since map name BPF_MAP_TYPE_CGROUP_STORAGE has been 
>>> used for old cgroup
>>> > > > > > > > local
>>> > > > > > > > storage support, the new map name 
>>> BPF_MAP_TYPE_CGROUP_LOCAL_STORAGE is
>>> > > > > > > > used
>>> > > > > > > > for cgroup storage available to non-cgroup-attached bpf 
>>> programs. The two
>>> > > > > > > > helpers are named as bpf_cgroup_local_storage_get() and
>>> > > > > > > > bpf_cgroup_local_storage_delete().
>>> > > > > > >
>>> > > > > > > Have you considered doing something similar to 
>>> 7d9c3427894f ("bpf: Make
>>> > > > > > > cgroup storages shared between programs on the same 
>>> cgroup") where
>>> > > > > > > the map changes its behavior depending on the key size 
>>> (see key_size checks
>>> > > > > > > in cgroup_storage_map_alloc)? Looks like sizeof(int) for 
>>> fd still
>>> > > > > > > can be used so we can, in theory, reuse the name..
>>> > > > > > >
>>> > > > > > > Pros:
>>> > > > > > > - no need for a new map name
>>> > > > > > >
>>> > > > > > > Cons:
>>> > > > > > > - existing BPF_MAP_TYPE_CGROUP_STORAGE is already messy; 
>>> might be not a
>>> > > > > > >     good idea to add more stuff to it?
>>> > > > > > >
>>> > > > > > > But, for the very least, should we also extend
>>> > > > > > > Documentation/bpf/map_cgroup_storage.rst to cover the new 
>>> map? We've
>>> > > > > > > tried to keep some of the important details in there..
>>> > > > > >
>>> > > > > > This might be a long shot, but is it possible to switch 
>>> completely to
>>> > > > > > this new generic cgroup storage, and for programs that 
>>> attach to
>>> > > > > > cgroups we can still do lookups/allocations during 
>>> attachment like we
>>> > > > > > do today? IOW, maintain the current API for cgroup progs 
>>> but switch it
>>> > > > > > to use this new map type instead.
>>> > > > > >
>>> > > > > > It feels like this map type is more generic and can be a 
>>> superset of
>>> > > > > > the existing cgroup storage, but I feel like I am missing 
>>> something.
>>> > > > >
>>> > > > > I feel like the biggest issue is that the existing
>>> > > > > bpf_get_local_storage helper is guaranteed to always return 
>>> non-null
>>> > > > > and the verifier doesn't require the programs to do null 
>>> checks on it;
>>> > > > > the new helper might return NULL making all existing programs 
>>> fail the
>>> > > > > verifier.
>>> > > >
>>> > > > What I meant is, keep the old bpf_get_local_storage helper only 
>>> for
>>> > > > cgroup-attached programs like we have today, and add a new generic
>>> > > > bpf_cgroup_local_storage_get() helper.
>>> > > >
>>> > > > For cgroup-attached programs, make sure a cgroup storage entry is
>>> > > > allocated and hooked to the helper on program attach time, to keep
>>> > > > today's behavior constant.
>>> > > >
>>> > > > For other programs, the bpf_cgroup_local_storage_get() will do the
>>> > > > normal lookup and allocate if necessary.
>>> > > >
>>> > > > Does this make any sense to you?
>>> > >
>>> > > But then you also need to somehow mark these to make sure it's not
>>> > > possible to delete them as long as the program is 
>>> loaded/attached? Not
>>> > > saying it's impossible, but it's a bit of a departure from the
>>> > > existing common local storage framework used by inode/task; not sure
>>> > > whether we want to pull all this complexity in there? But we can
>>> > > definitely try if there is a wider agreement..
>>> >
>>> > I agree that it's not ideal, but it feels like we are comparing two
>>> > non-ideal options anyway, I am just throwing ideas around :)
>>
>>> I don't think it is a good idea to marry the new
>>> BPF_MAP_TYPE_CGROUP_LOCAL_STORAGE and the existing
>>> BPF_MAP_TYPE_CGROUP_STORAGE in any way.  The API is very different.  
>>> A few
>>> have already been mentioned here.  Delete is one.  Storage creation 
>>> time is
>>> another one.  The map key is also different.  Yes, maybe we can reuse 
>>> the
>>> different key size concept in bpf_cgroup_storage_key in some way but 
>>> still
>>> feel too much unnecessary quirks for the existing sk/inode/task storage
>>> users to remember.
>>
>>> imo, it is better to keep them separate and have a different map-type.
>>> Adding a map flag or using map extra will make it sounds like an 
>>> extension
>>> which it is not.
>>
>> This part is the most confusing to me:
>>
>> BPF_MAP_TYPE_CGROUP_STORAGE       bpf_get_local_storage
>> BPF_MAP_TYPE_CGROUP_LOCAL_STORAGE bpf_cgroup_local_storage_get
>>
>> The new helpers should probably drop 'local' name to match the 
>> task/inode ([0])?
>> And we're left with:
>>
>> BPF_MAP_TYPE_CGROUP_STORAGE       bpf_get_local_storage
>> BPF_MAP_TYPE_CGROUP_LOCAL_STORAGE bpf_cgroup_storage_get
>>
>> You read CGROUP_STORAGE via get_local_storage and
>> you read CGROUP_LOCAL_STORAGE via cgroup_storage_get :-/
> 
> Yep, agree that it is not ideal :(

I guess I need to add more documentation to explain the difference
of old and new map regardless of the final names.

> 
>>
>> That's why I'm slightly tilting towards reusing the name. At least we can
>> add a big DEPRECATED message for bpf_get_local_storage and that seems 
>> to be
>> it? All those extra key sizes can also be deprecated, but I'm honestly
>> not sure if anybody is using them.
> 
> Reusing 'key_size == sizeof(int)' to mean new map type...hmm...  I have 
> been thinking about it after your suggestion in another reply since it 
> can use the BPF_MAP_TYPE_CGROUP_STORAGE name.  I wish the 
> BPF_MAP_TYPE_CGROUP_LOCAL_STORAGE was given to the 
> bpf_get_local_storage() instead because it is a better name to describe 
> what it is doing.
> 
> hmm.... However, this feels working like a map_flags or map_extra but in 
> a more hidden way.  I am worry it will actually be more confusing and 
> also having usage surprises when there are quite many behavior 
> differences that this thread has already mentioned.  That will be hard 
> for the user to reason those API differences just because of using a 
> different key_size.
> 
> May be going back to revisit the naming a little bit.  How about giving 
> a new and likely more correct 'BPF_MAP_TYPE_CGRP_LOCAL_STORAGE' name for 
> the existing bpf_get_local_storage() use.  Then
> 
> '#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.

Another alternative is to name the map name as
     BPF_MAP_TYPE_CGROUP_STORAGE2
to indicate it is a different version of cgroup_storage map
and the documentation should explain the difference clearly.
This should avoid the possible confusion between
BPF_MAP_TYPE_CGROUP_STORAGE and BPF_MAP_TYPE_CGRP_STORAGE.

> 
>>
>> But having a separate map also seems fine, as long as we have a patch to
>> update the existing header documentation. (and mention in
>> Documentation/bpf/map_cgroup_storage.rst that there is a replacement?)
>> Current bpf_get_local_storage description is too vague; let's at least
>> mention that it works only with BPF_MAP_TYPE_CGROUP_STORAGE.
>>
>> 0: 
>> https://lore.kernel.org/bpf/6ce7d490-f015-531f-3dbb-b6f7717f0590@meta.com/T/#mb2107250caa19a8d9ec3549a52f4a9698be99e33
>>
>>> > >
>>> > > > > There might be something else I don't remember at this point 
>>> (besides
>>> > > > > that weird per-prog_type that we'd have to emulate as well)..
>>> > > >
>>> > > > Yeah there are things that will need to be emulated, but I feel 
>>> like
>>> > > > we may end up with less confusing code (and less code in general).
>>
>>
>