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

[sdf@google.com]

On 10/17, Yonghong Song wrote:


> 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.

+1, CGRP_STORAGE sounds good!

> 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).
> > >
> > >
> >