Re: [PATCH bpf-next v3] bpf: increase supported cgroup storage value size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 26, 2021 at 4:00 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> Current max cgroup storage value size is 4k (PAGE_SIZE). The other local
> storages accept up to 64k (BPF_LOCAL_STORAGE_MAX_VALUE_SIZE). Let's align
> max cgroup value size with the other storages.
>
> For percpu, the max is 32k (PCPU_MIN_UNIT_SIZE) because percpu
> allocator is not happy about larger values.
>
> netcnt test is extended to exercise those maximum values
> (non-percpu max size is close to, but not real max).
>
> v3:
> * refine SIZEOF_BPF_LOCAL_STORAGE_ELEM comment (Yonghong Song)
> * anonymous struct in percpu_net_cnt & net_cnt (Yonghong Song)
> * reorder free (Yonghong Song)
>
> v2:
> * cap max_value_size instead of BUILD_BUG_ON (Martin KaFai Lau)
>
> Cc: Martin KaFai Lau <kafai@xxxxxx>
> Cc: Yonghong Song <yhs@xxxxxx>
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
>  kernel/bpf/local_storage.c                  | 11 +++++-
>  tools/testing/selftests/bpf/netcnt_common.h | 38 +++++++++++++++++----
>  tools/testing/selftests/bpf/test_netcnt.c   | 17 ++++++---
>  3 files changed, 53 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 7ed2a14dc0de..035e9e3a7132 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -1,6 +1,7 @@
>  //SPDX-License-Identifier: GPL-2.0
>  #include <linux/bpf-cgroup.h>
>  #include <linux/bpf.h>
> +#include <linux/bpf_local_storage.h>
>  #include <linux/btf.h>
>  #include <linux/bug.h>
>  #include <linux/filter.h>
> @@ -283,9 +284,17 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
>
>  static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr)
>  {
> +       __u32 max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
>         int numa_node = bpf_map_attr_numa_node(attr);
>         struct bpf_cgroup_storage_map *map;
>
> +       /* percpu is bound by PCPU_MIN_UNIT_SIZE, non-percu
> +        * is the same as other local storages.
> +        */
> +       if (attr->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> +               max_value_size = min_t(__u32, max_value_size,
> +                                      PCPU_MIN_UNIT_SIZE);
> +
>         if (attr->key_size != sizeof(struct bpf_cgroup_storage_key) &&
>             attr->key_size != sizeof(__u64))
>                 return ERR_PTR(-EINVAL);
> @@ -293,7 +302,7 @@ static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr)
>         if (attr->value_size == 0)
>                 return ERR_PTR(-EINVAL);
>
> -       if (attr->value_size > PAGE_SIZE)
> +       if (attr->value_size > max_value_size)
>                 return ERR_PTR(-E2BIG);
>
>         if (attr->map_flags & ~LOCAL_STORAGE_CREATE_FLAG_MASK ||
> diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> index 81084c1c2c23..87f5b97e1932 100644
> --- a/tools/testing/selftests/bpf/netcnt_common.h
> +++ b/tools/testing/selftests/bpf/netcnt_common.h
> @@ -6,19 +6,43 @@
>
>  #define MAX_PERCPU_PACKETS 32
>
> +/* sizeof(struct bpf_local_storage_elem):
> + *
> + * It really is about 128 bytes on x86_64, but allocate more to account for
> + * possible layout changes, different architectures, etc.
> + * The kernel will wrap up to PAGE_SIZE internally anyway.
> + */
> +#define SIZEOF_BPF_LOCAL_STORAGE_ELEM          256
> +
> +/* Try to estimate kernel's BPF_LOCAL_STORAGE_MAX_VALUE_SIZE: */
> +#define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE       (0xFFFF - \
> +                                                SIZEOF_BPF_LOCAL_STORAGE_ELEM)
> +
> +#define PCPU_MIN_UNIT_SIZE                     32768
> +
>  struct percpu_net_cnt {
> -       __u64 packets;
> -       __u64 bytes;
> +       union {

so you have a struct with a single anonymous union inside, isn't that
right? Any problems with just making struct percpu_net_cnt into union
percpu_net_cnt?

> +               struct {
> +                       __u64 packets;
> +                       __u64 bytes;
>
> -       __u64 prev_ts;
> +                       __u64 prev_ts;
>
> -       __u64 prev_packets;
> -       __u64 prev_bytes;
> +                       __u64 prev_packets;
> +                       __u64 prev_bytes;
> +               };
> +               __u8 data[PCPU_MIN_UNIT_SIZE];
> +       };
>  };
>
>  struct net_cnt {
> -       __u64 packets;
> -       __u64 bytes;
> +       union {

similarly here

> +               struct {
> +                       __u64 packets;
> +                       __u64 bytes;
> +               };
> +               __u8 data[BPF_LOCAL_STORAGE_MAX_VALUE_SIZE];
> +       };
>  };
>
>  #endif
> diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> index a7b9a69f4fd5..372afccf2d17 100644
> --- a/tools/testing/selftests/bpf/test_netcnt.c
> +++ b/tools/testing/selftests/bpf/test_netcnt.c
> @@ -33,11 +33,11 @@ static int bpf_find_map(const char *test, struct bpf_object *obj,
>
>  int main(int argc, char **argv)
>  {
> -       struct percpu_net_cnt *percpu_netcnt;
> +       struct percpu_net_cnt *percpu_netcnt = NULL;
>         struct bpf_cgroup_storage_key key;
> +       struct net_cnt *netcnt = NULL;
>         int map_fd, percpu_map_fd;
>         int error = EXIT_FAILURE;
> -       struct net_cnt netcnt;
>         struct bpf_object *obj;
>         int prog_fd, cgroup_fd;
>         unsigned long packets;
> @@ -52,6 +52,12 @@ int main(int argc, char **argv)
>                 goto err;
>         }
>
> +       netcnt = malloc(sizeof(*netcnt));

curious, was it too big to be just allocated on the stack? Isn't the
thread stack size much bigger than 64KB (at least by default)?

> +       if (!netcnt) {
> +               printf("Not enough memory for non-per-cpu area\n");
> +               goto err;
> +       }
> +
>         if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
>                           &obj, &prog_fd)) {
>                 printf("Failed to load bpf program\n");
> @@ -96,7 +102,7 @@ int main(int argc, char **argv)
>                 goto err;
>         }
>
> -       if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> +       if (bpf_map_lookup_elem(map_fd, &key, netcnt)) {
>                 printf("Failed to lookup cgroup storage\n");
>                 goto err;
>         }
> @@ -109,8 +115,8 @@ int main(int argc, char **argv)
>         /* Some packets can be still in per-cpu cache, but not more than
>          * MAX_PERCPU_PACKETS.
>          */
> -       packets = netcnt.packets;
> -       bytes = netcnt.bytes;
> +       packets = netcnt->packets;
> +       bytes = netcnt->bytes;
>         for (cpu = 0; cpu < nproc; cpu++) {
>                 if (percpu_netcnt[cpu].packets > MAX_PERCPU_PACKETS) {
>                         printf("Unexpected percpu value: %llu\n",
> @@ -141,6 +147,7 @@ int main(int argc, char **argv)
>
>  err:
>         cleanup_cgroup_environment();
> +       free(netcnt);
>         free(percpu_netcnt);
>
>  out:
> --
> 2.32.0.432.gabb21c7263-goog
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux