On 07/27, Andrii Nakryiko wrote:

On Tue, Jul 27, 2021 at 1:47 PM [off-list ref] wrote:

quoted

On 07/27, Andrii Nakryiko wrote:

quoted

On Mon, Jul 26, 2021 at 4:00 PM Stanislav Fomichev [off-list ref]  

wrote:

quoted

quoted

quoted

Current max cgroup storage value size is 4k (PAGE_SIZE). The other  

local

quoted

quoted

quoted

storages accept up to 64k (BPF_LOCAL_STORAGE_MAX_VALUE_SIZE). Let's

align

quoted

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 <redacted>
Cc: Yonghong Song <redacted>
Signed-off-by: Stanislav Fomichev <redacted>
---
 kernel/bpf/local_storage.c                  | 11 +++++-
 tools/testing/selftests/bpf/netcnt_common.h | 38  

+++++++++++++++++----

quoted

quoted

quoted

 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,

quoted

 static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr  

*attr)

quoted

quoted

quoted

 {
+       __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)  

&&

quoted

quoted

quoted

            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)

quoted

        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

quoted

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

quoted

+ * 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)

quoted

+
+#define PCPU_MIN_UNIT_SIZE                     32768
+
 struct percpu_net_cnt {
-       __u64 packets;
-       __u64 bytes;
+       union {

quoted

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?

We'd have to s/struct/union/ everywhere in this case, not sure
we want to add more churn? Seemed easier to do anonymous union+struct.

4 occurrences for net_cnt and another 4 for percpu_net_cnt, not much
churn (and all pretty localized). But I honestly don't care, just
wanted to note that you don't need this extra nesting.

I might do it since I'm doing another respin anyway.

quoted

quoted

quoted

+               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 {

quoted

similarly here

quoted

quoted

+               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

quoted

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,

quoted

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

quoted

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

I haven't tried really, I just moved it to malloc because it crossed
some unconscious boundary for the 'stuff I allocate on the stack'.
I can try it out if you prefer to keep it on the stack, let me know.

Yeah, if it can stay on the stack. Less thinking about freeing memory.

Ack, seems to be working, will resend shortly..