* Re: [PATCH bpf-next] bpf: increase supported cgroup storage value size
@ 2021-07-23 10:39 kernel test robot
0 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-07-23 10:39 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 3735 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210723002747.3668098-1-sdf@google.com>
References: <20210723002747.3668098-1-sdf@google.com>
TO: Stanislav Fomichev <sdf@google.com>
TO: netdev(a)vger.kernel.org
TO: bpf(a)vger.kernel.org
CC: ast(a)kernel.org
CC: daniel(a)iogearbox.net
CC: andrii(a)kernel.org
CC: Stanislav Fomichev <sdf@google.com>
Hi Stanislav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Stanislav-Fomichev/bpf-increase-supported-cgroup-storage-value-size/20210723-083012
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
:::::: branch date: 10 hours ago
:::::: commit date: 10 hours ago
config: x86_64-randconfig-b001-20210723 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# apt-get install iwyu # include-what-you-use
# https://github.com/0day-ci/linux/commit/137ee089a79dff33cbaa40509b43a9f820df4524
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Stanislav-Fomichev/bpf-increase-supported-cgroup-storage-value-size/20210723-083012
git checkout 137ee089a79dff33cbaa40509b43a9f820df4524
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross C=1 CHECK=iwyu O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/bpf/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
iwyu warnings: (new ones prefixed by >>)
kernel/bpf/local_storage.c:2:1: iwyu: warning: superfluous #include <linux/bpf-cgroup.h>
kernel/bpf/local_storage.c:3:1: iwyu: warning: superfluous #include <linux/bpf.h>
>> kernel/bpf/local_storage.c:4:1: iwyu: warning: superfluous #include <linux/bpf_local_storage.h>
kernel/bpf/local_storage.c:5:1: iwyu: warning: superfluous #include <linux/btf.h>
kernel/bpf/local_storage.c:6:1: iwyu: warning: superfluous #include <linux/bug.h>
kernel/bpf/local_storage.c:7:1: iwyu: warning: superfluous #include <linux/filter.h>
kernel/bpf/local_storage.c:8:1: iwyu: warning: superfluous #include <linux/mm.h>
kernel/bpf/local_storage.c:9:1: iwyu: warning: superfluous #include <linux/rbtree.h>
kernel/bpf/local_storage.c:10:1: iwyu: warning: superfluous #include <linux/slab.h>
kernel/bpf/local_storage.c:11:1: iwyu: warning: superfluous #include <uapi/linux/btf.h>
vim +4 kernel/bpf/local_storage.c
137ee089a79dff Stanislav Fomichev 2021-07-22 @4 #include <linux/bpf_local_storage.h>
9a1126b63190e2 Roman Gushchin 2018-12-10 5 #include <linux/btf.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 6 #include <linux/bug.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 7 #include <linux/filter.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 8 #include <linux/mm.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 9 #include <linux/rbtree.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 10 #include <linux/slab.h>
9a1126b63190e2 Roman Gushchin 2018-12-10 11 #include <uapi/linux/btf.h>
de9cbbaadba5ad Roman Gushchin 2018-08-02 12
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28900 bytes --]
^ permalink raw reply [flat|nested] 4+ 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; 4+ 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] 4+ messages in thread
* Re: [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
2021-07-23 23:23 ` sdf
0 siblings, 1 reply; 4+ 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] 4+ messages in thread
* [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; 4+ 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 related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-23 23:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 10:39 [PATCH bpf-next] bpf: increase supported cgroup storage value size kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-07-23 0:27 Stanislav Fomichev
2021-07-23 22:39 ` Martin KaFai Lau
2021-07-23 23:23 ` sdf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.