| Message ID | 20210727222335.4029096-1-sdf@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 33b57e0cc78eb82f2921eb4c6d1c8fcaa733823b |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v4] bpf: increase supported cgroup storage value size | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: linux-kselftest@vger.kernel.org kpsingh@kernel.org john.fastabend@gmail.com songliubraving@fb.com shuah@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 121 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
Hello: This patch was applied to bpf/bpf-next.git (refs/heads/master): On Tue, 27 Jul 2021 15:23:35 -0700 you 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. > > [...] Here is the summary with links: - [bpf-next,v4] bpf: increase supported cgroup storage value size https://git.kernel.org/bpf/bpf-next/c/33b57e0cc78e You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Tue, Jul 27, 2021 at 3:23 PM Stanislav Fomichev <sdf@google.com> 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). > > v4: > * remove inner union (Andrii Nakryiko) > * keep net_cnt on the stack (Andrii Nakryiko) > > 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@fb.com> > Cc: Yonghong Song <yhs@fb.com> > Cc: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- Added Martin's ack and applied to bpf-next. Please carry over received Acks between revisions. It's also a good practice to separate selftest from the kernel (or libbpf) changes, unless kernel change doesn't immediately break selftest. Please consider doing that for the future. I also just noticed that test_netcnt isn't part of test_progs. It would be great to migrate it under the common test_progs infrastructure. We've been steadily moving towards that, but there are still a bunch of tests that are not run in CI. > kernel/bpf/local_storage.c | 11 +++++- > tools/testing/selftests/bpf/netcnt_common.h | 38 ++++++++++++++----- > .../testing/selftests/bpf/progs/netcnt_prog.c | 8 ++-- > tools/testing/selftests/bpf/test_netcnt.c | 4 +- > 4 files changed, 45 insertions(+), 16 deletions(-) > [...]
On 07/27, Andrii Nakryiko wrote: > On Tue, Jul 27, 2021 at 3:23 PM Stanislav Fomichev <sdf@google.com> 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). > > > > v4: > > * remove inner union (Andrii Nakryiko) > > * keep net_cnt on the stack (Andrii Nakryiko) > > > > 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@fb.com> > > Cc: Yonghong Song <yhs@fb.com> > > Cc: Andrii Nakryiko <andrii@kernel.org> > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > Added Martin's ack and applied to bpf-next. Please carry over received > Acks between revisions. Ah, sorry, forgot about it :-( > It's also a good practice to separate selftest from the kernel (or > libbpf) changes, unless kernel change doesn't immediately break > selftest. Please consider doing that for the future. I've actually seen some back and forth on this one. I used to split them in the past (assuming it makes it easy to do the backports/cherry-picks), but I remember at some point it was suggested not to split them for small changes like this. Might be a good idea to document this (when and if to separate libbpf/selftests) on bpf_devel_QA.rst > I also just noticed that test_netcnt isn't part of test_progs. It > would be great to migrate it under the common test_progs > infrastructure. We've been steadily moving towards that, but there are > still a bunch of tests that are not run in CI. SG, I might do a follow up on this one.
On Tue, Jul 27, 2021 at 4:25 PM <sdf@google.com> wrote: > > On 07/27, Andrii Nakryiko wrote: > > On Tue, Jul 27, 2021 at 3:23 PM Stanislav Fomichev <sdf@google.com> 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). > > > > > > v4: > > > * remove inner union (Andrii Nakryiko) > > > * keep net_cnt on the stack (Andrii Nakryiko) > > > > > > 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@fb.com> > > > Cc: Yonghong Song <yhs@fb.com> > > > Cc: Andrii Nakryiko <andrii@kernel.org> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > Added Martin's ack and applied to bpf-next. Please carry over received > > Acks between revisions. > Ah, sorry, forgot about it :-( > > > It's also a good practice to separate selftest from the kernel (or > > libbpf) changes, unless kernel change doesn't immediately break > > selftest. Please consider doing that for the future. > I've actually seen some back and forth on this one. I used to split > them in the past (assuming it makes it easy to do the > backports/cherry-picks), but I remember at some point it was > suggested not to split them for small changes like this. So we asked to split UAPI header sync for tools/include/ into a separate patch initially. But then I just improved libbpf's sync script to handle that regardless and we stopped asking for that. But the libbpf vs kernel vs selftests split was always (perhaps implicitly) advised. Personally, I've only had a few cases where selftest changes had to go in with kernel changes in the same patch to avoid breaking selftests. In all other cases it's cleaner to have them split out. > > Might be a good idea to document this (when and if to separate > libbpf/selftests) > on bpf_devel_QA.rst > > > I also just noticed that test_netcnt isn't part of test_progs. It > > would be great to migrate it under the common test_progs > > infrastructure. We've been steadily moving towards that, but there are > > still a bunch of tests that are not run in CI. > SG, I might do a follow up on this one. Sounds good, thanks!
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..0ab1c88041cd 100644 --- a/tools/testing/selftests/bpf/netcnt_common.h +++ b/tools/testing/selftests/bpf/netcnt_common.h @@ -6,19 +6,39 @@ #define MAX_PERCPU_PACKETS 32 -struct percpu_net_cnt { - __u64 packets; - __u64 bytes; +/* 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 - __u64 prev_ts; +/* Try to estimate kernel's BPF_LOCAL_STORAGE_MAX_VALUE_SIZE: */ +#define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE (0xFFFF - \ + SIZEOF_BPF_LOCAL_STORAGE_ELEM) - __u64 prev_packets; - __u64 prev_bytes; +#define PCPU_MIN_UNIT_SIZE 32768 + +union percpu_net_cnt { + struct { + __u64 packets; + __u64 bytes; + + __u64 prev_ts; + + __u64 prev_packets; + __u64 prev_bytes; + }; + __u8 data[PCPU_MIN_UNIT_SIZE]; }; -struct net_cnt { - __u64 packets; - __u64 bytes; +union net_cnt { + struct { + __u64 packets; + __u64 bytes; + }; + __u8 data[BPF_LOCAL_STORAGE_MAX_VALUE_SIZE]; }; #endif diff --git a/tools/testing/selftests/bpf/progs/netcnt_prog.c b/tools/testing/selftests/bpf/progs/netcnt_prog.c index d071adf178bd..43649bce4c54 100644 --- a/tools/testing/selftests/bpf/progs/netcnt_prog.c +++ b/tools/testing/selftests/bpf/progs/netcnt_prog.c @@ -13,21 +13,21 @@ struct { __uint(type, BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE); __type(key, struct bpf_cgroup_storage_key); - __type(value, struct percpu_net_cnt); + __type(value, union percpu_net_cnt); } percpu_netcnt SEC(".maps"); struct { __uint(type, BPF_MAP_TYPE_CGROUP_STORAGE); __type(key, struct bpf_cgroup_storage_key); - __type(value, struct net_cnt); + __type(value, union net_cnt); } netcnt SEC(".maps"); SEC("cgroup/skb") int bpf_nextcnt(struct __sk_buff *skb) { - struct percpu_net_cnt *percpu_cnt; + union percpu_net_cnt *percpu_cnt; char fmt[] = "%d %llu %llu\n"; - struct net_cnt *cnt; + union net_cnt *cnt; __u64 ts, dt; int ret; diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c index a7b9a69f4fd5..4990a99e7381 100644 --- a/tools/testing/selftests/bpf/test_netcnt.c +++ b/tools/testing/selftests/bpf/test_netcnt.c @@ -33,14 +33,14 @@ static int bpf_find_map(const char *test, struct bpf_object *obj, int main(int argc, char **argv) { - struct percpu_net_cnt *percpu_netcnt; + union percpu_net_cnt *percpu_netcnt; struct bpf_cgroup_storage_key key; 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; + union net_cnt netcnt; unsigned long bytes; int cpu, nproc; __u32 prog_cnt;
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). v4: * remove inner union (Andrii Nakryiko) * keep net_cnt on the stack (Andrii Nakryiko) 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@fb.com> Cc: Yonghong Song <yhs@fb.com> Cc: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- kernel/bpf/local_storage.c | 11 +++++- tools/testing/selftests/bpf/netcnt_common.h | 38 ++++++++++++++----- .../testing/selftests/bpf/progs/netcnt_prog.c | 8 ++-- tools/testing/selftests/bpf/test_netcnt.c | 4 +- 4 files changed, 45 insertions(+), 16 deletions(-)