Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: increase supported cgroup storage value size
@ 2021-07-23  0:27 Stanislav Fomichev
  2021-07-23 22:39 ` Martin KaFai Lau
  0 siblings, 1 reply; 3+ messages in thread
From: Stanislav Fomichev @ 2021-07-23  0:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev

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

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/local_storage.c                    | 12 +++++-
 tools/testing/selftests/bpf/netcnt_common.h   | 38 +++++++++++++++----
 .../testing/selftests/bpf/progs/netcnt_prog.c | 29 +++++++-------
 tools/testing/selftests/bpf/test_netcnt.c     | 25 +++++++-----
 4 files changed, 73 insertions(+), 31 deletions(-)

diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index 7ed2a14dc0de..a276da74c20a 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>
@@ -284,8 +285,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)
 {
 	int numa_node = bpf_map_attr_numa_node(attr);
+	__u32 max_value_size = PCPU_MIN_UNIT_SIZE;
 	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_CGROUP_STORAGE)
+		max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
+
+	BUILD_BUG_ON(PCPU_MIN_UNIT_SIZE > BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
+
 	if (attr->key_size != sizeof(struct bpf_cgroup_storage_key) &&
 	    attr->key_size != sizeof(__u64))
 		return ERR_PTR(-EINVAL);
@@ -293,7 +303,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..dfcf184ff713 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, but allocate more to account for possible
+ * layout changes, different architectures, etc.
+ * It 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 {
+		struct {
+			__u64 packets;
+			__u64 bytes;
 
-	__u64 prev_ts;
+			__u64 prev_ts;
 
-	__u64 prev_packets;
-	__u64 prev_bytes;
+			__u64 prev_packets;
+			__u64 prev_bytes;
+		} val;
+		__u8 data[PCPU_MIN_UNIT_SIZE];
+	};
 };
 
 struct net_cnt {
-	__u64 packets;
-	__u64 bytes;
+	union {
+		struct {
+			__u64 packets;
+			__u64 bytes;
+		} val;
+		__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..4b0884239892 100644
--- a/tools/testing/selftests/bpf/progs/netcnt_prog.c
+++ b/tools/testing/selftests/bpf/progs/netcnt_prog.c
@@ -34,34 +34,35 @@ int bpf_nextcnt(struct __sk_buff *skb)
 	cnt = bpf_get_local_storage(&netcnt, 0);
 	percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
 
-	percpu_cnt->packets++;
-	percpu_cnt->bytes += skb->len;
+	percpu_cnt->val.packets++;
+	percpu_cnt->val.bytes += skb->len;
 
-	if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
-		__sync_fetch_and_add(&cnt->packets,
-				     percpu_cnt->packets);
-		percpu_cnt->packets = 0;
+	if (percpu_cnt->val.packets > MAX_PERCPU_PACKETS) {
+		__sync_fetch_and_add(&cnt->val.packets,
+				     percpu_cnt->val.packets);
+		percpu_cnt->val.packets = 0;
 
-		__sync_fetch_and_add(&cnt->bytes,
-				     percpu_cnt->bytes);
-		percpu_cnt->bytes = 0;
+		__sync_fetch_and_add(&cnt->val.bytes,
+				     percpu_cnt->val.bytes);
+		percpu_cnt->val.bytes = 0;
 	}
 
 	ts = bpf_ktime_get_ns();
-	dt = ts - percpu_cnt->prev_ts;
+	dt = ts - percpu_cnt->val.prev_ts;
 
 	dt *= MAX_BPS;
 	dt /= NS_PER_SEC;
 
-	if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
+	if (cnt->val.bytes + percpu_cnt->val.bytes -
+	    percpu_cnt->val.prev_bytes < dt)
 		ret = 1;
 	else
 		ret = 0;
 
 	if (dt > REFRESH_TIME_NS) {
-		percpu_cnt->prev_ts = ts;
-		percpu_cnt->prev_packets = cnt->packets;
-		percpu_cnt->prev_bytes = cnt->bytes;
+		percpu_cnt->val.prev_ts = ts;
+		percpu_cnt->val.prev_packets = cnt->val.packets;
+		percpu_cnt->val.prev_bytes = cnt->val.bytes;
 	}
 
 	return !!ret;
diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
index a7b9a69f4fd5..1138765406a5 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));
+	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,17 +115,17 @@ 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->val.packets;
+	bytes = netcnt->val.bytes;
 	for (cpu = 0; cpu < nproc; cpu++) {
-		if (percpu_netcnt[cpu].packets > MAX_PERCPU_PACKETS) {
+		if (percpu_netcnt[cpu].val.packets > MAX_PERCPU_PACKETS) {
 			printf("Unexpected percpu value: %llu\n",
-			       percpu_netcnt[cpu].packets);
+			       percpu_netcnt[cpu].val.packets);
 			goto err;
 		}
 
-		packets += percpu_netcnt[cpu].packets;
-		bytes += percpu_netcnt[cpu].bytes;
+		packets += percpu_netcnt[cpu].val.packets;
+		bytes += percpu_netcnt[cpu].val.bytes;
 	}
 
 	/* No packets should be lost */
@@ -142,6 +148,7 @@ int main(int argc, char **argv)
 err:
 	cleanup_cgroup_environment();
 	free(percpu_netcnt);
+	free(netcnt);
 
 out:
 	return error;
-- 
2.32.0.432.gabb21c7263-goog


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

* Re: [PATCH bpf-next] bpf: increase supported cgroup storage value size
  2021-07-23  0:27 [PATCH bpf-next] bpf: increase supported cgroup storage value size Stanislav Fomichev
@ 2021-07-23 22:39 ` Martin KaFai Lau
  2021-07-23 23:23   ` sdf
  0 siblings, 1 reply; 3+ messages in thread
From: Martin KaFai Lau @ 2021-07-23 22:39 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, ast, daniel, andrii

On Thu, Jul 22, 2021 at 05:27:47PM -0700, Stanislav Fomichev 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).
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  kernel/bpf/local_storage.c                    | 12 +++++-
>  tools/testing/selftests/bpf/netcnt_common.h   | 38 +++++++++++++++----
>  .../testing/selftests/bpf/progs/netcnt_prog.c | 29 +++++++-------
>  tools/testing/selftests/bpf/test_netcnt.c     | 25 +++++++-----
>  4 files changed, 73 insertions(+), 31 deletions(-)
> 
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 7ed2a14dc0de..a276da74c20a 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>
> @@ -284,8 +285,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)
>  {
>  	int numa_node = bpf_map_attr_numa_node(attr);
> +	__u32 max_value_size = PCPU_MIN_UNIT_SIZE;
>  	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_CGROUP_STORAGE)
> +		max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
> +
> +	BUILD_BUG_ON(PCPU_MIN_UNIT_SIZE > BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
If PCPU_MIN_UNIT_SIZE did become larger, I assume it would be bounded by
BPF_LOCAL_STORAGE_MAX_VALUE_SIZE again?

Instead of BUILD_BUG_ON, how about a min_t here:

	if (attr->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
		max_value_size = min_t(__u32,
					BPF_LOCAL_STORAGE_MAX_VALUE_SIZE,
					PCPU_MIN_UNIT_SIZE);

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

* Re: [PATCH bpf-next] bpf: increase supported cgroup storage value size
  2021-07-23 22:39 ` Martin KaFai Lau
@ 2021-07-23 23:23   ` sdf
  0 siblings, 0 replies; 3+ messages in thread
From: sdf @ 2021-07-23 23:23 UTC (permalink / raw)
  To: Martin KaFai Lau; +Cc: netdev, bpf, ast, daniel, andrii

On 07/23, Martin KaFai Lau wrote:
> On Thu, Jul 22, 2021 at 05:27:47PM -0700, Stanislav Fomichev 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).
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  kernel/bpf/local_storage.c                    | 12 +++++-
> >  tools/testing/selftests/bpf/netcnt_common.h   | 38 +++++++++++++++----
> >  .../testing/selftests/bpf/progs/netcnt_prog.c | 29 +++++++-------
> >  tools/testing/selftests/bpf/test_netcnt.c     | 25 +++++++-----
> >  4 files changed, 73 insertions(+), 31 deletions(-)
> >
> > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> > index 7ed2a14dc0de..a276da74c20a 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>
> > @@ -284,8 +285,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)
> >  {
> >  	int numa_node = bpf_map_attr_numa_node(attr);
> > +	__u32 max_value_size = PCPU_MIN_UNIT_SIZE;
> >  	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_CGROUP_STORAGE)
> > +		max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
> > +
> > +	BUILD_BUG_ON(PCPU_MIN_UNIT_SIZE > BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
> If PCPU_MIN_UNIT_SIZE did become larger, I assume it would be bounded by
> BPF_LOCAL_STORAGE_MAX_VALUE_SIZE again?

> Instead of BUILD_BUG_ON, how about a min_t here:

> 	if (attr->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> 		max_value_size = min_t(__u32,
> 					BPF_LOCAL_STORAGE_MAX_VALUE_SIZE,
> 					PCPU_MIN_UNIT_SIZE);

Sounds like a much better idea, thanks, will incorporate into v2.

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

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

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23  0:27 [PATCH bpf-next] bpf: increase supported cgroup storage value size Stanislav Fomichev
2021-07-23 22:39 ` Martin KaFai Lau
2021-07-23 23:23   ` sdf

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