On Thu, Oct 20, 2022 at 03:13:27PM -0700, Yonghong Song wrote:
> Add some descriptions and examples for BPF_MAP_TYPE_CGRP_STROAGE.
> Also illustate the major difference between BPF_MAP_TYPE_CGRP_STROAGE
> and BPF_MAP_TYPE_CGROUP_STORAGE and recommend to use
> BPF_MAP_TYPE_CGRP_STROAGE instead of BPF_MAP_TYPE_CGROUP_STORAGE
> in the end.
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> Documentation/bpf/map_cgrp_storage.rst | 104 +++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
> create mode 100644 Documentation/bpf/map_cgrp_storage.rst
>
> diff --git a/Documentation/bpf/map_cgrp_storage.rst b/Documentation/bpf/map_cgrp_storage.rst
> new file mode 100644
> index 000000000000..15691aab7fc7
> --- /dev/null
> +++ b/Documentation/bpf/map_cgrp_storage.rst
> @@ -0,0 +1,104 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +.. Copyright (C) 2022 Meta Platforms, Inc. and affiliates.
> +
> +===========================
> +BPF_MAP_TYPE_CGRP_STORAGE
> +===========================
> +
> +The ``BPF_MAP_TYPE_CGRP_STORAGE`` map type represents a local fix-sized
> +storage for cgroups. It is only available with ``CONFIG_CGROUP_BPF``.
> +The programs are made available by the same Kconfig. The
> +data for a particular cgroup can be retrieved by looking up the map
> +with that cgroup.
> +
> +This document describes the usage and semantics of the
> +``BPF_MAP_TYPE_CGRP_STORAGE`` map type.
> +
> +Usage
> +=====
> +
> +The map key must be ``sizeof(int)`` representing a cgroup fd.
> +To access the storage in a program, use ``bpf_cgrp_storage_get``::
> +
> + void *bpf_cgrp_storage_get(struct bpf_map *map, struct cgroup *cgroup, void *value, u64 flags)
> +
> +``flags`` could be 0 or ``BPF_LOCAL_STORAGE_GET_F_CREATE`` which indicates that
> +a new local storage will be created if one does not exist.
> +
> +The local storage can be removed with ``bpf_cgrp_storage_delete``::
> +
> + long bpf_cgrp_storage_delete(struct bpf_map *map, struct cgruop *cgroup)
s/cgruop/cgroup
> +
> +The map is available to all program types.
> +
> +Examples
> +========
> +
> +An bpf-program example with BPF_MAP_TYPE_CGRP_STORAGE::
s/An bpf-program/A bpf program
> +
> + #include <vmlinux.h>
> + #include <bpf/bpf_helpers.h>
> + #include <bpf/bpf_tracing.h>
> +
> + struct {
> + __uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> + __uint(map_flags, BPF_F_NO_PREALLOC);
> + __type(key, int);
> + __type(value, long);
> + } cgrp_storage SEC(".maps");
> +
> + SEC("tp_btf/sys_enter")
> + int BPF_PROG(on_enter, struct pt_regs *regs, long id)
> + {
> + struct task_struct *task = bpf_get_current_task_btf();
> + long *ptr;
> +
> + ptr = bpf_cgrp_storage_get(&cgrp_storage, task->cgroups->dfl_cgrp, 0,
> + BPF_LOCAL_STORAGE_GET_F_CREATE);
> + if (ptr)
> + __sync_fetch_and_add(ptr, 1);
> +
> + return 0;
> + }
> +
> +Userspace accessing map declared above::
> +
> + #include <linux/bpf.h>
> + #include <linux/libbpf.h>
> +
> + __u32 map_lookup(struct bpf_map *map, int cgrp_fd)
> + {
> + __u32 *value;
> + value = bpf_map_lookup_elem(bpf_map__fd(map), &cgrp_fd);
> + if (value)
> + return *value;
> + return 0;
> + }
> +
> +Difference Between BPF_MAP_TYPE_CGRP_STORAGE and BPF_MAP_TYPE_CGROUP_STORAGE
> +============================================================================
> +
> +The main difference between ``BPF_MAP_TYPE_CGRP_STORAGE`` and ``BPF_MAP_TYPE_CGROUP_STORAGE``
> +is that ``BPF_MAP_TYPE_CGRP_STORAGE`` can be used by all program types while
> +``BPF_MAP_TYPE_CGROUP_STORAGE`` is available only to cgroup program types.
Suggestion: I'd consider going into just a bit more detail about what's
meant by "cgroup program types" here. Maybe just 1-2 sentences
describing the types of programs where the deprecated cgroup storage map
type is available, and why. A program that's using the
BPF_MAP_TYPE_CGRP_STORAGE map is conceptually also a "cgroup program
type" in that it's querying, analyzing, etc cgroups, so being explicit
may help get ahead of any confusion.
Also, given that BPF_MAP_TYPE_CGROUP_STORAGE is deprecated, and is
extremely close in name to BPF_MAP_TYPE_CGRP_STORAGE, perhaps we should
start this section out by explaining that BPF_MAP_TYPE_CGROUP_STORAGE is
a deprecated map type that's now aliased to
BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED, and then reference it as
BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED for the rest of the page. I think
it's important to highlight that the map type is deprecated, and give
some historical context here so that users understand why they should
use BPF_MAP_TYPE_CGRP_STORAGE, and why we have two such confusingly-
similarly named maps. What do you think?
> +There are some other differences as well.
> +
> +(1). ``BPF_MAP_TYPE_CGRP_STORAGE`` supports local storage for more than one
> + cgroups while ``BPF_MAP_TYPE_CGROUP_STORAGE`` only support one, the one attached
s/cgroups/cgroup
> + by the bpf program.
> +
> +(2). ``BPF_MAP_TYPE_CGROUP_STORAGE`` allocates local storage at attach time so
> + ``bpf_get_local_storage()`` always returns non-null local storage.
Suggestion: s/non-null/non-NULL. In general, I'd suggest changing null
to NULL throughout the page, but I don't feel strongly about it.
> + ``BPF_MAP_TYPE_CGRP_STORAGE`` allocates local storage at runtime so
> + it is possible that ``bpf_cgrp_storage_get()`` may return null local storage.
> + To avoid such null local storage issue, user space can do
> + ``bpf_map_update_elem()`` to pre-allocate local storage.
Should we specify that user space can preallocate by doing
bpf_map_update_elem() _before_ the program is attached? Also, another
small nit, but I think pre-allocate and de-allocate should just be
preallocate and deallocate respectively.
> +(3). ``BPF_MAP_TYPE_CGRP_STORAGE`` supports de-allocating local storage by bpf program
s/by bpf program/by a bpf program
> + while ``BPF_MAP_TYPE_CGROUP_STORAGE`` only de-allocates storage during
> + prog detach time.
> +
> +So overall, ``BPF_MAP_TYPE_CGRP_STORAGE`` supports all ``BPF_MAP_TYPE_CGROUP_STORAGE``
> +functionality and beyond. It is recommended to use ``BPF_MAP_TYPE_CGRP_STORAGE``
> +instead of ``BPF_MAP_TYPE_CGROUP_STORAGE``.
As mentioned above, I think we need to go beyond stating that using
BPF_MAP_TYPE_CGRP_STORAGE is recommended, and also explicitly and loudly
call out that BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED is deprecated and
will not be supported indefinitely.
Overall though, this looks great. Thank you for writing it up.
Thanks,
David
