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

Yonghong Song <yhs@xxxxxxxx> · Mon, 17 Oct 2022 22:59:43 -0700

On 10/17/22 5:52 PM, Martin KaFai Lau wrote:
On 10/17/22 3:16 PM, sdf@xxxxxxxxxx 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@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 17, 2022 at 11:47 AM Yosry Ahmed 
<yosryahmed@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 17, 2022 at 11:43 AM Stanislav Fomichev 
<sdf@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Oct 17, 2022 at 11:26 AM Yosry Ahmed 
<yosryahmed@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Oct 17, 2022 at 11:02 AM <sdf@xxxxxxxxxx> 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@xxxxxxxx/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).