On Sun, Oct 23, 2022 at 11:05:52AM -0700, Yonghong Song wrote:
> Add some descriptions and examples for BPF_MAP_TYPE_CGRP_STROAGE.
s/STROAGE/STORAGE here and elsewhere
> 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 | 109 +++++++++++++++++++++++++
> 1 file changed, 109 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..4dfc7770da7e
> --- /dev/null
> +++ b/Documentation/bpf/map_cgrp_storage.rst
> @@ -0,0 +1,109 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +.. Copyright (C) 2022 Meta Platforms, Inc. and affiliates.
> +
> +===========================
> +BPF_MAP_TYPE_CGRP_STORAGE
> +===========================
Small nit, can you trim the == border so it matches the width of
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``.
This is no longer accurate and should say, "It is only available with
``CONFIG_CGROUPS``."
> +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 cgroup *cgroup)
> +
> +The map is available to all program types.
> +
> +Examples
> +========
> +
> +A bpf program example with BPF_MAP_TYPE_CGRP_STORAGE::
> +
> + #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 old cgroup storage map ``BPF_MAP_TYPE_CGROUP_STORAGE`` has been marked as
> +deprecated (renamed to ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED``). The new
> +``BPF_MAP_TYPE_CGRP_STORAGE`` map should be used instead. The following
> +illusates the main difference between ``BPF_MAP_TYPE_CGRP_STORAGE`` and
> +``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED``.
> +
> +(1). ``BPF_MAP_TYPE_CGRP_STORAGE`` can be used by all program types while
> + ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED`` is available only to cgroup program types
> + like BPF_CGROUP_INET_INGRESS or BPF_CGROUP_SOCK_OPS, etc.
> +
> +(2). ``BPF_MAP_TYPE_CGRP_STORAGE`` supports local storage for more than one
> + cgroup while ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED`` only support one cgroup
s/only support/only supports
> + which is attached by a bpf program.
> +
> +(3). ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED`` allocates local storage at attach time so
> + ``bpf_get_local_storage()`` always returns non-NULL local storage.
> + ``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 before a bpf program
> + is attached.
> +
> +(4). ``BPF_MAP_TYPE_CGRP_STORAGE`` supports deleting local storage by a bpf program
> + while ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED`` only deletes storage during
> + prog detach time.
> +
> +So overall, ``BPF_MAP_TYPE_CGRP_STORAGE`` supports all ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED``
> +functionality and beyond. It is recommended to use ``BPF_MAP_TYPE_CGRP_STORAGE``
> +instead of ``BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED``.
> --
> 2.30.2
>
One more thought / consideration which you can of course feel free to
ignore. If we're using the acronym "bpf" in documentation (acronym
meaning it's used on its own rather than e.g. in a variable name), IMO
we should capitalize it to "BPF". Very minor thing, but I think it makes
the docs look a bit more polished. Up to you, and not a big deal either
way.
Anyways, this looks great, thanks again for writing it up! Everything I
pointed out was a minor typo / fix so:
Acked-by: David Vernet <void@xxxxxxxxxxxxx>
