Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf-next v4] bpf: increase supported cgroup storage value size
@ 2021-07-27 22:23 Stanislav Fomichev
  2021-07-27 23:00 ` patchwork-bot+netdevbpf
  2021-07-27 23:00 ` Andrii Nakryiko
  0 siblings, 2 replies; 5+ messages in thread
From: Stanislav Fomichev @ 2021-07-27 22:23 UTC (permalink / raw)
  To: netdev, bpf
  Cc: ast, daniel, andrii, Stanislav Fomichev, Martin KaFai Lau, Yonghong Song

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(-)

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;
-- 
2.32.0.554.ge1b32706d8-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size
  2021-07-27 22:23 [PATCH bpf-next v4] bpf: increase supported cgroup storage value size Stanislav Fomichev
@ 2021-07-27 23:00 ` patchwork-bot+netdevbpf
  2021-07-27 23:00 ` Andrii Nakryiko
  1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-07-27 23:00 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, ast, daniel, andrii, kafai, yhs

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size
  2021-07-27 22:23 [PATCH bpf-next v4] bpf: increase supported cgroup storage value size Stanislav Fomichev
  2021-07-27 23:00 ` patchwork-bot+netdevbpf
@ 2021-07-27 23:00 ` Andrii Nakryiko
  2021-07-27 23:25   ` sdf
  1 sibling, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2021-07-27 23:00 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Yonghong Song

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(-)
>

[...]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size
  2021-07-27 23:00 ` Andrii Nakryiko
@ 2021-07-27 23:25   ` sdf
  2021-07-27 23:48     ` Andrii Nakryiko
  0 siblings, 1 reply; 5+ messages in thread
From: sdf @ 2021-07-27 23:25 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Yonghong Song

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size
  2021-07-27 23:25   ` sdf
@ 2021-07-27 23:48     ` Andrii Nakryiko
  0 siblings, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2021-07-27 23:48 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Yonghong Song

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!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-27 23:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 22:23 [PATCH bpf-next v4] bpf: increase supported cgroup storage value size Stanislav Fomichev
2021-07-27 23:00 ` patchwork-bot+netdevbpf
2021-07-27 23:00 ` Andrii Nakryiko
2021-07-27 23:25   ` sdf
2021-07-27 23:48     ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).