* [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
@ 2022-05-16 2:24 Feng zhou
2022-05-17 3:09 ` Yonghong Song
2022-05-19 0:17 ` Andrii Nakryiko
0 siblings, 2 replies; 7+ messages in thread
From: Feng zhou @ 2022-05-16 2:24 UTC (permalink / raw)
To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
kpsingh, rostedt, mingo, jolsa, davemarchevsky, joannekoong,
geliang.tang
Cc: netdev, bpf, linux-kernel, duanxiongchun, songmuchun,
wangdongdong.6, cong.wang, zhouchengming, zhoufeng.zf,
yosryahmed
From: Feng Zhou <zhoufeng.zf@bytedance.com>
comments from Andrii Nakryiko, details in here:
https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
use /* */ instead of //
use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
use 8 bytes for value size
fix memory leak
use ASSERT_EQ instead of ASSERT_OK
add bpf_loop to fetch values on each possible CPU
Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
---
.../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
.../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
2 files changed, 70 insertions(+), 40 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
index 58b24c2112b0..89ca170f1c25 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
@@ -1,30 +1,39 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2022 Bytedance
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2022 Bytedance */
#include <test_progs.h>
#include "test_map_lookup_percpu_elem.skel.h"
-#define TEST_VALUE 1
-
void test_map_lookup_percpu_elem(void)
{
struct test_map_lookup_percpu_elem *skel;
- int key = 0, ret;
- int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
- int *buf;
+ __u64 key = 0, sum;
+ int ret, i;
+ int nr_cpus = libbpf_num_possible_cpus();
+ __u64 *buf;
- buf = (int *)malloc(nr_cpus*sizeof(int));
+ buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
if (!ASSERT_OK_PTR(buf, "malloc"))
return;
- memset(buf, 0, nr_cpus*sizeof(int));
- buf[0] = TEST_VALUE;
- skel = test_map_lookup_percpu_elem__open_and_load();
- if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
- return;
+ for (i=0; i<nr_cpus; i++)
+ buf[i] = i;
+ sum = (nr_cpus-1)*nr_cpus/2;
+
+ skel = test_map_lookup_percpu_elem__open();
+ if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
+ goto exit;
+
+ skel->rodata->nr_cpus = nr_cpus;
+
+ ret = test_map_lookup_percpu_elem__load(skel);
+ if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
+ goto cleanup;
+
ret = test_map_lookup_percpu_elem__attach(skel);
- ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
+ if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
+ goto cleanup;
ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
ASSERT_OK(ret, "percpu_array_map update");
@@ -37,10 +46,14 @@ void test_map_lookup_percpu_elem(void)
syscall(__NR_getuid);
- ret = skel->bss->percpu_array_elem_val == TEST_VALUE &&
- skel->bss->percpu_hash_elem_val == TEST_VALUE &&
- skel->bss->percpu_lru_hash_elem_val == TEST_VALUE;
- ASSERT_OK(!ret, "bpf_map_lookup_percpu_elem success");
+ test_map_lookup_percpu_elem__detach(skel);
+
+ ASSERT_EQ(skel->bss->percpu_array_elem_sum, sum, "percpu_array lookup percpu elem");
+ ASSERT_EQ(skel->bss->percpu_hash_elem_sum, sum, "percpu_hash lookup percpu elem");
+ ASSERT_EQ(skel->bss->percpu_lru_hash_elem_sum, sum, "percpu_lru_hash lookup percpu elem");
+cleanup:
test_map_lookup_percpu_elem__destroy(skel);
+exit:
+ free(buf);
}
diff --git a/tools/testing/selftests/bpf/progs/test_map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/progs/test_map_lookup_percpu_elem.c
index 5d4ef86cbf48..75479180571f 100644
--- a/tools/testing/selftests/bpf/progs/test_map_lookup_percpu_elem.c
+++ b/tools/testing/selftests/bpf/progs/test_map_lookup_percpu_elem.c
@@ -1,52 +1,69 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2022 Bytedance
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2022 Bytedance */
#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
-int percpu_array_elem_val = 0;
-int percpu_hash_elem_val = 0;
-int percpu_lru_hash_elem_val = 0;
+__u64 percpu_array_elem_sum = 0;
+__u64 percpu_hash_elem_sum = 0;
+__u64 percpu_lru_hash_elem_sum = 0;
+const volatile int nr_cpus;
struct {
__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
__uint(max_entries, 1);
__type(key, __u32);
- __type(value, __u32);
+ __type(value, __u64);
} percpu_array_map SEC(".maps");
struct {
__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
__uint(max_entries, 1);
- __type(key, __u32);
- __type(value, __u32);
+ __type(key, __u64);
+ __type(value, __u64);
} percpu_hash_map SEC(".maps");
struct {
__uint(type, BPF_MAP_TYPE_LRU_PERCPU_HASH);
__uint(max_entries, 1);
- __type(key, __u32);
- __type(value, __u32);
+ __type(key, __u64);
+ __type(value, __u64);
} percpu_lru_hash_map SEC(".maps");
+struct read_percpu_elem_ctx {
+ void *map;
+ __u64 sum;
+};
+
+static int read_percpu_elem_callback(__u32 index, struct read_percpu_elem_ctx *ctx)
+{
+ __u64 key = 0;
+ __u64 *value;
+ value = bpf_map_lookup_percpu_elem(ctx->map, &key, index);
+ if (value)
+ ctx->sum += *value;
+ return 0;
+}
+
SEC("tp/syscalls/sys_enter_getuid")
int sysenter_getuid(const void *ctx)
{
- __u32 key = 0;
- __u32 cpu = 0;
- __u32 *value;
+ struct read_percpu_elem_ctx map_ctx;
- value = bpf_map_lookup_percpu_elem(&percpu_array_map, &key, cpu);
- if (value)
- percpu_array_elem_val = *value;
+ map_ctx.map = &percpu_array_map;
+ map_ctx.sum = 0;
+ bpf_loop(nr_cpus, read_percpu_elem_callback, &map_ctx, 0);
+ percpu_array_elem_sum = map_ctx.sum;
- value = bpf_map_lookup_percpu_elem(&percpu_hash_map, &key, cpu);
- if (value)
- percpu_hash_elem_val = *value;
+ map_ctx.map = &percpu_hash_map;
+ map_ctx.sum = 0;
+ bpf_loop(nr_cpus, read_percpu_elem_callback, &map_ctx, 0);
+ percpu_hash_elem_sum = map_ctx.sum;
- value = bpf_map_lookup_percpu_elem(&percpu_lru_hash_map, &key, cpu);
- if (value)
- percpu_lru_hash_elem_val = *value;
+ map_ctx.map = &percpu_lru_hash_map;
+ map_ctx.sum = 0;
+ bpf_loop(nr_cpus, read_percpu_elem_callback, &map_ctx, 0);
+ percpu_lru_hash_elem_sum = map_ctx.sum;
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-16 2:24 [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase Feng zhou
@ 2022-05-17 3:09 ` Yonghong Song
2022-05-17 7:05 ` [External] " Feng Zhou
2022-05-19 0:17 ` Andrii Nakryiko
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2022-05-17 3:09 UTC (permalink / raw)
To: Feng zhou, ast, daniel, andrii, kafai, songliubraving,
john.fastabend, kpsingh, rostedt, mingo, jolsa, davemarchevsky,
joannekoong, geliang.tang
Cc: netdev, bpf, linux-kernel, duanxiongchun, songmuchun,
wangdongdong.6, cong.wang, zhouchengming, yosryahmed
On 5/15/22 7:24 PM, Feng zhou wrote:
> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>
> comments from Andrii Nakryiko, details in here:
> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
>
> use /* */ instead of //
> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
> use 8 bytes for value size
> fix memory leak
> use ASSERT_EQ instead of ASSERT_OK
> add bpf_loop to fetch values on each possible CPU
>
> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
LGTM with a few nits below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
> 2 files changed, 70 insertions(+), 40 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> index 58b24c2112b0..89ca170f1c25 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> @@ -1,30 +1,39 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2022 Bytedance
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2022 Bytedance */
>
> #include <test_progs.h>
>
The above empty line is unnecessary.
> #include "test_map_lookup_percpu_elem.skel.h"
>
> -#define TEST_VALUE 1
> -
> void test_map_lookup_percpu_elem(void)
> {
> struct test_map_lookup_percpu_elem *skel;
> - int key = 0, ret;
> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> - int *buf;
> + __u64 key = 0, sum;
> + int ret, i;
> + int nr_cpus = libbpf_num_possible_cpus();
> + __u64 *buf;
>
> - buf = (int *)malloc(nr_cpus*sizeof(int));
> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
> if (!ASSERT_OK_PTR(buf, "malloc"))
> return;
> - memset(buf, 0, nr_cpus*sizeof(int));
> - buf[0] = TEST_VALUE;
>
> - skel = test_map_lookup_percpu_elem__open_and_load();
> - if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
> - return;
> + for (i=0; i<nr_cpus; i++)
> + buf[i] = i;
> + sum = (nr_cpus-1)*nr_cpus/2;
> +
> + skel = test_map_lookup_percpu_elem__open();
> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
> + goto exit;
> +
> + skel->rodata->nr_cpus = nr_cpus;
> +
> + ret = test_map_lookup_percpu_elem__load(skel);
> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
> + goto cleanup;
> +
> ret = test_map_lookup_percpu_elem__attach(skel);
> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
> + goto cleanup;
>
> ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
> ASSERT_OK(ret, "percpu_array_map update");
> @@ -37,10 +46,14 @@ void test_map_lookup_percpu_elem(void)
>
> syscall(__NR_getuid);
>
> - ret = skel->bss->percpu_array_elem_val == TEST_VALUE &&
> - skel->bss->percpu_hash_elem_val == TEST_VALUE &&
> - skel->bss->percpu_lru_hash_elem_val == TEST_VALUE;
> - ASSERT_OK(!ret, "bpf_map_lookup_percpu_elem success");
> + test_map_lookup_percpu_elem__detach(skel);
> +
> + ASSERT_EQ(skel->bss->percpu_array_elem_sum, sum, "percpu_array lookup percpu elem");
> + ASSERT_EQ(skel->bss->percpu_hash_elem_sum, sum, "percpu_hash lookup percpu elem");
> + ASSERT_EQ(skel->bss->percpu_lru_hash_elem_sum, sum, "percpu_lru_hash lookup percpu elem");
>
> +cleanup:
> test_map_lookup_percpu_elem__destroy(skel);
> +exit:
> + free(buf);
> }
[...]
> +struct read_percpu_elem_ctx {
> + void *map;
> + __u64 sum;
> +};
> +
> +static int read_percpu_elem_callback(__u32 index, struct read_percpu_elem_ctx *ctx)
> +{
> + __u64 key = 0;
> + __u64 *value;
Please add an empty line here.
> + value = bpf_map_lookup_percpu_elem(ctx->map, &key, index);
> + if (value)
> + ctx->sum += *value;
> + return 0;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [External] Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-17 3:09 ` Yonghong Song
@ 2022-05-17 7:05 ` Feng Zhou
0 siblings, 0 replies; 7+ messages in thread
From: Feng Zhou @ 2022-05-17 7:05 UTC (permalink / raw)
To: Yonghong Song, ast, daniel, andrii, kafai, songliubraving,
john.fastabend, kpsingh, rostedt, mingo, jolsa, davemarchevsky,
joannekoong, geliang.tang
Cc: netdev, bpf, linux-kernel, duanxiongchun, songmuchun,
wangdongdong.6, cong.wang, zhouchengming, yosryahmed
在 2022/5/17 上午11:09, Yonghong Song 写道:
>
>
> On 5/15/22 7:24 PM, Feng zhou wrote:
>> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>>
>> comments from Andrii Nakryiko, details in here:
>> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
>>
>>
>> use /* */ instead of //
>> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
>> use 8 bytes for value size
>> fix memory leak
>> use ASSERT_EQ instead of ASSERT_OK
>> add bpf_loop to fetch values on each possible CPU
>>
>> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add
>> test case for bpf_map_lookup_percpu_elem")
>> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
>
> LGTM with a few nits below.
> Acked-by: Yonghong Song <yhs@fb.com>
>
Ok, will do. Thanks.
>> ---
>> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
>> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
>> 2 files changed, 70 insertions(+), 40 deletions(-)
>>
>> diff --git
>> a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> index 58b24c2112b0..89ca170f1c25 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> @@ -1,30 +1,39 @@
>> -// SPDX-License-Identifier: GPL-2.0
>> -// Copyright (c) 2022 Bytedance
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2022 Bytedance */
>> #include <test_progs.h>
>
> The above empty line is unnecessary.
>
>> #include "test_map_lookup_percpu_elem.skel.h"
>> -#define TEST_VALUE 1
>> -
>> void test_map_lookup_percpu_elem(void)
>> {
>> struct test_map_lookup_percpu_elem *skel;
>> - int key = 0, ret;
>> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>> - int *buf;
>> + __u64 key = 0, sum;
>> + int ret, i;
>> + int nr_cpus = libbpf_num_possible_cpus();
>> + __u64 *buf;
>> - buf = (int *)malloc(nr_cpus*sizeof(int));
>> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
>> if (!ASSERT_OK_PTR(buf, "malloc"))
>> return;
>> - memset(buf, 0, nr_cpus*sizeof(int));
>> - buf[0] = TEST_VALUE;
>> - skel = test_map_lookup_percpu_elem__open_and_load();
>> - if (!ASSERT_OK_PTR(skel,
>> "test_map_lookup_percpu_elem__open_and_load"))
>> - return;
>> + for (i=0; i<nr_cpus; i++)
>> + buf[i] = i;
>> + sum = (nr_cpus-1)*nr_cpus/2;
>> +
>> + skel = test_map_lookup_percpu_elem__open();
>> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
>> + goto exit;
>> +
>> + skel->rodata->nr_cpus = nr_cpus;
>> +
>> + ret = test_map_lookup_percpu_elem__load(skel);
>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
>> + goto cleanup;
>> +
>> ret = test_map_lookup_percpu_elem__attach(skel);
>> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
>> + goto cleanup;
>> ret =
>> bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key,
>> buf, 0);
>> ASSERT_OK(ret, "percpu_array_map update");
>> @@ -37,10 +46,14 @@ void test_map_lookup_percpu_elem(void)
>> syscall(__NR_getuid);
>> - ret = skel->bss->percpu_array_elem_val == TEST_VALUE &&
>> - skel->bss->percpu_hash_elem_val == TEST_VALUE &&
>> - skel->bss->percpu_lru_hash_elem_val == TEST_VALUE;
>> - ASSERT_OK(!ret, "bpf_map_lookup_percpu_elem success");
>> + test_map_lookup_percpu_elem__detach(skel);
>> +
>> + ASSERT_EQ(skel->bss->percpu_array_elem_sum, sum, "percpu_array
>> lookup percpu elem");
>> + ASSERT_EQ(skel->bss->percpu_hash_elem_sum, sum, "percpu_hash
>> lookup percpu elem");
>> + ASSERT_EQ(skel->bss->percpu_lru_hash_elem_sum, sum,
>> "percpu_lru_hash lookup percpu elem");
>> +cleanup:
>> test_map_lookup_percpu_elem__destroy(skel);
>> +exit:
>> + free(buf);
>> }
> [...]
>> +struct read_percpu_elem_ctx {
>> + void *map;
>> + __u64 sum;
>> +};
>> +
>> +static int read_percpu_elem_callback(__u32 index, struct
>> read_percpu_elem_ctx *ctx)
>> +{
>> + __u64 key = 0;
>> + __u64 *value;
>
> Please add an empty line here.
>
>> + value = bpf_map_lookup_percpu_elem(ctx->map, &key, index);
>> + if (value)
>> + ctx->sum += *value;
>> + return 0;
>> +}
>> +
> [...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-16 2:24 [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase Feng zhou
2022-05-17 3:09 ` Yonghong Song
@ 2022-05-19 0:17 ` Andrii Nakryiko
2022-05-19 3:26 ` [External] " Feng Zhou
1 sibling, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2022-05-19 0:17 UTC (permalink / raw)
To: Feng zhou
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh,
Steven Rostedt, Ingo Molnar, Jiri Olsa, Dave Marchevsky,
Joanne Koong, Geliang Tang, Networking, bpf, open list,
duanxiongchun, Muchun Song, Dongdong Wang, Cong Wang,
zhouchengming, Yosry Ahmed
On Sun, May 15, 2022 at 7:25 PM Feng zhou <zhoufeng.zf@bytedance.com> wrote:
>
> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>
> comments from Andrii Nakryiko, details in here:
> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
>
> use /* */ instead of //
> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
> use 8 bytes for value size
> fix memory leak
> use ASSERT_EQ instead of ASSERT_OK
> add bpf_loop to fetch values on each possible CPU
>
> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
> ---
> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
> 2 files changed, 70 insertions(+), 40 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> index 58b24c2112b0..89ca170f1c25 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> @@ -1,30 +1,39 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2022 Bytedance
> +/* SPDX-License-Identifier: GPL-2.0 */
heh, so for SPDX license comment the rule is to use // in .c files :)
so keep SPDX as // and all others as /* */
> +/* Copyright (c) 2022 Bytedance */
>
> #include <test_progs.h>
>
> #include "test_map_lookup_percpu_elem.skel.h"
>
> -#define TEST_VALUE 1
> -
> void test_map_lookup_percpu_elem(void)
> {
> struct test_map_lookup_percpu_elem *skel;
> - int key = 0, ret;
> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> - int *buf;
> + __u64 key = 0, sum;
> + int ret, i;
> + int nr_cpus = libbpf_num_possible_cpus();
> + __u64 *buf;
>
> - buf = (int *)malloc(nr_cpus*sizeof(int));
> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
no need for casting
> if (!ASSERT_OK_PTR(buf, "malloc"))
> return;
> - memset(buf, 0, nr_cpus*sizeof(int));
> - buf[0] = TEST_VALUE;
>
> - skel = test_map_lookup_percpu_elem__open_and_load();
> - if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
> - return;
> + for (i=0; i<nr_cpus; i++)
spaces between operators
> + buf[i] = i;
> + sum = (nr_cpus-1)*nr_cpus/2;
same, please follow kernel code style
> +
> + skel = test_map_lookup_percpu_elem__open();
> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
> + goto exit;
> +
nit: keep it simple, init skel to NULL and use single cleanup goto
label that will destroy skel unconditionally (it deals with NULL just
fine)
> + skel->rodata->nr_cpus = nr_cpus;
> +
> + ret = test_map_lookup_percpu_elem__load(skel);
> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
> + goto cleanup;
> +
> ret = test_map_lookup_percpu_elem__attach(skel);
> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
> + goto cleanup;
>
> ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
> ASSERT_OK(ret, "percpu_array_map update");
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [External] Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-19 0:17 ` Andrii Nakryiko
@ 2022-05-19 3:26 ` Feng Zhou
2022-05-19 4:38 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Feng Zhou @ 2022-05-19 3:26 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh,
Steven Rostedt, Ingo Molnar, Jiri Olsa, Dave Marchevsky,
Joanne Koong, Geliang Tang, Networking, bpf, open list,
duanxiongchun, Muchun Song, Dongdong Wang, Cong Wang,
zhouchengming, Yosry Ahmed
在 2022/5/19 上午8:17, Andrii Nakryiko 写道:
> On Sun, May 15, 2022 at 7:25 PM Feng zhou <zhoufeng.zf@bytedance.com> wrote:
>> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>>
>> comments from Andrii Nakryiko, details in here:
>> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
>>
>> use /* */ instead of //
>> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
>> use 8 bytes for value size
>> fix memory leak
>> use ASSERT_EQ instead of ASSERT_OK
>> add bpf_loop to fetch values on each possible CPU
>>
>> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
>> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
>> ---
>> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
>> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
>> 2 files changed, 70 insertions(+), 40 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> index 58b24c2112b0..89ca170f1c25 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>> @@ -1,30 +1,39 @@
>> -// SPDX-License-Identifier: GPL-2.0
>> -// Copyright (c) 2022 Bytedance
>> +/* SPDX-License-Identifier: GPL-2.0 */
> heh, so for SPDX license comment the rule is to use // in .c files :)
> so keep SPDX as // and all others as /* */
will do. Thanks.
>
>> +/* Copyright (c) 2022 Bytedance */
>>
>> #include <test_progs.h>
>>
>> #include "test_map_lookup_percpu_elem.skel.h"
>>
>> -#define TEST_VALUE 1
>> -
>> void test_map_lookup_percpu_elem(void)
>> {
>> struct test_map_lookup_percpu_elem *skel;
>> - int key = 0, ret;
>> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>> - int *buf;
>> + __u64 key = 0, sum;
>> + int ret, i;
>> + int nr_cpus = libbpf_num_possible_cpus();
>> + __u64 *buf;
>>
>> - buf = (int *)malloc(nr_cpus*sizeof(int));
>> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
> no need for casting
casting means no '(__u64 *)'?
just like this:
'buf = malloc(nr_cpus * sizeof(__u64));'
>
>> if (!ASSERT_OK_PTR(buf, "malloc"))
>> return;
>> - memset(buf, 0, nr_cpus*sizeof(int));
>> - buf[0] = TEST_VALUE;
>>
>> - skel = test_map_lookup_percpu_elem__open_and_load();
>> - if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
>> - return;
>> + for (i=0; i<nr_cpus; i++)
> spaces between operators
will do. Thanks.
>
>> + buf[i] = i;
>> + sum = (nr_cpus-1)*nr_cpus/2;
> same, please follow kernel code style
will do. Thanks.
>
>> +
>> + skel = test_map_lookup_percpu_elem__open();
>> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
>> + goto exit;
>> +
> nit: keep it simple, init skel to NULL and use single cleanup goto
> label that will destroy skel unconditionally (it deals with NULL just
> fine)
will do. Thanks.
>> + skel->rodata->nr_cpus = nr_cpus;
>> +
>> + ret = test_map_lookup_percpu_elem__load(skel);
>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
>> + goto cleanup;
>> +
>> ret = test_map_lookup_percpu_elem__attach(skel);
>> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
>> + goto cleanup;
>>
>> ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
>> ASSERT_OK(ret, "percpu_array_map update");
> [...]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [External] Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-19 3:26 ` [External] " Feng Zhou
@ 2022-05-19 4:38 ` Andrii Nakryiko
2022-05-19 7:31 ` Feng Zhou
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2022-05-19 4:38 UTC (permalink / raw)
To: Feng Zhou
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh,
Steven Rostedt, Ingo Molnar, Jiri Olsa, Dave Marchevsky,
Joanne Koong, Geliang Tang, Networking, bpf, open list,
duanxiongchun, Muchun Song, Dongdong Wang, Cong Wang,
zhouchengming, Yosry Ahmed
On Wed, May 18, 2022 at 8:27 PM Feng Zhou <zhoufeng.zf@bytedance.com> wrote:
>
> 在 2022/5/19 上午8:17, Andrii Nakryiko 写道:
> > On Sun, May 15, 2022 at 7:25 PM Feng zhou <zhoufeng.zf@bytedance.com> wrote:
> >> From: Feng Zhou <zhoufeng.zf@bytedance.com>
> >>
> >> comments from Andrii Nakryiko, details in here:
> >> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
> >>
> >> use /* */ instead of //
> >> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
> >> use 8 bytes for value size
> >> fix memory leak
> >> use ASSERT_EQ instead of ASSERT_OK
> >> add bpf_loop to fetch values on each possible CPU
> >>
> >> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
> >> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
> >> ---
> >> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
> >> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
> >> 2 files changed, 70 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> >> index 58b24c2112b0..89ca170f1c25 100644
> >> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> >> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> >> @@ -1,30 +1,39 @@
> >> -// SPDX-License-Identifier: GPL-2.0
> >> -// Copyright (c) 2022 Bytedance
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> > heh, so for SPDX license comment the rule is to use // in .c files :)
> > so keep SPDX as // and all others as /* */
>
> will do. Thanks.
>
> >
> >> +/* Copyright (c) 2022 Bytedance */
> >>
> >> #include <test_progs.h>
> >>
> >> #include "test_map_lookup_percpu_elem.skel.h"
> >>
> >> -#define TEST_VALUE 1
> >> -
> >> void test_map_lookup_percpu_elem(void)
> >> {
> >> struct test_map_lookup_percpu_elem *skel;
> >> - int key = 0, ret;
> >> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> >> - int *buf;
> >> + __u64 key = 0, sum;
> >> + int ret, i;
> >> + int nr_cpus = libbpf_num_possible_cpus();
> >> + __u64 *buf;
> >>
> >> - buf = (int *)malloc(nr_cpus*sizeof(int));
> >> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
> > no need for casting
>
> casting means no '(__u64 *)'?
> just like this:
> 'buf = malloc(nr_cpus * sizeof(__u64));'
>
yes, in C you don't need to explicitly cast void * to other pointer types
> >
> >> if (!ASSERT_OK_PTR(buf, "malloc"))
> >> return;
> >> - memset(buf, 0, nr_cpus*sizeof(int));
> >> - buf[0] = TEST_VALUE;
> >>
> >> - skel = test_map_lookup_percpu_elem__open_and_load();
> >> - if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
> >> - return;
> >> + for (i=0; i<nr_cpus; i++)
> > spaces between operators
>
> will do. Thanks.
>
> >
> >> + buf[i] = i;
> >> + sum = (nr_cpus-1)*nr_cpus/2;
> > same, please follow kernel code style
>
> will do. Thanks.
>
> >
> >> +
> >> + skel = test_map_lookup_percpu_elem__open();
> >> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
> >> + goto exit;
> >> +
> > nit: keep it simple, init skel to NULL and use single cleanup goto
> > label that will destroy skel unconditionally (it deals with NULL just
> > fine)
>
> will do. Thanks.
>
> >> + skel->rodata->nr_cpus = nr_cpus;
> >> +
> >> + ret = test_map_lookup_percpu_elem__load(skel);
> >> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
> >> + goto cleanup;
> >> +
> >> ret = test_map_lookup_percpu_elem__attach(skel);
> >> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
> >> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
> >> + goto cleanup;
> >>
> >> ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
> >> ASSERT_OK(ret, "percpu_array_map update");
> > [...]
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [External] Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
2022-05-19 4:38 ` Andrii Nakryiko
@ 2022-05-19 7:31 ` Feng Zhou
0 siblings, 0 replies; 7+ messages in thread
From: Feng Zhou @ 2022-05-19 7:31 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh,
Steven Rostedt, Ingo Molnar, Jiri Olsa, Dave Marchevsky,
Joanne Koong, Geliang Tang, Networking, bpf, open list,
duanxiongchun, Muchun Song, Dongdong Wang, Cong Wang,
zhouchengming, Yosry Ahmed
在 2022/5/19 下午12:38, Andrii Nakryiko 写道:
> On Wed, May 18, 2022 at 8:27 PM Feng Zhou <zhoufeng.zf@bytedance.com> wrote:
>> 在 2022/5/19 上午8:17, Andrii Nakryiko 写道:
>>> On Sun, May 15, 2022 at 7:25 PM Feng zhou <zhoufeng.zf@bytedance.com> wrote:
>>>> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>>>>
>>>> comments from Andrii Nakryiko, details in here:
>>>> https://lore.kernel.org/lkml/20220511093854.411-1-zhoufeng.zf@bytedance.com/T/
>>>>
>>>> use /* */ instead of //
>>>> use libbpf_num_possible_cpus() instead of sysconf(_SC_NPROCESSORS_ONLN)
>>>> use 8 bytes for value size
>>>> fix memory leak
>>>> use ASSERT_EQ instead of ASSERT_OK
>>>> add bpf_loop to fetch values on each possible CPU
>>>>
>>>> Fixes: ed7c13776e20c74486b0939a3c1de984c5efb6aa ("selftests/bpf: add test case for bpf_map_lookup_percpu_elem")
>>>> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
>>>> ---
>>>> .../bpf/prog_tests/map_lookup_percpu_elem.c | 49 +++++++++------
>>>> .../bpf/progs/test_map_lookup_percpu_elem.c | 61 ++++++++++++-------
>>>> 2 files changed, 70 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>>>> index 58b24c2112b0..89ca170f1c25 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
>>>> @@ -1,30 +1,39 @@
>>>> -// SPDX-License-Identifier: GPL-2.0
>>>> -// Copyright (c) 2022 Bytedance
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> heh, so for SPDX license comment the rule is to use // in .c files :)
>>> so keep SPDX as // and all others as /* */
>> will do. Thanks.
>>
>>>> +/* Copyright (c) 2022 Bytedance */
>>>>
>>>> #include <test_progs.h>
>>>>
>>>> #include "test_map_lookup_percpu_elem.skel.h"
>>>>
>>>> -#define TEST_VALUE 1
>>>> -
>>>> void test_map_lookup_percpu_elem(void)
>>>> {
>>>> struct test_map_lookup_percpu_elem *skel;
>>>> - int key = 0, ret;
>>>> - int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>>>> - int *buf;
>>>> + __u64 key = 0, sum;
>>>> + int ret, i;
>>>> + int nr_cpus = libbpf_num_possible_cpus();
>>>> + __u64 *buf;
>>>>
>>>> - buf = (int *)malloc(nr_cpus*sizeof(int));
>>>> + buf = (__u64 *)malloc(nr_cpus*sizeof(__u64));
>>> no need for casting
>> casting means no '(__u64 *)'?
>> just like this:
>> 'buf = malloc(nr_cpus * sizeof(__u64));'
>>
> yes, in C you don't need to explicitly cast void * to other pointer types
Ok, Thanks.
>
>>>> if (!ASSERT_OK_PTR(buf, "malloc"))
>>>> return;
>>>> - memset(buf, 0, nr_cpus*sizeof(int));
>>>> - buf[0] = TEST_VALUE;
>>>>
>>>> - skel = test_map_lookup_percpu_elem__open_and_load();
>>>> - if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open_and_load"))
>>>> - return;
>>>> + for (i=0; i<nr_cpus; i++)
>>> spaces between operators
>> will do. Thanks.
>>
>>>> + buf[i] = i;
>>>> + sum = (nr_cpus-1)*nr_cpus/2;
>>> same, please follow kernel code style
>> will do. Thanks.
>>
>>>> +
>>>> + skel = test_map_lookup_percpu_elem__open();
>>>> + if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
>>>> + goto exit;
>>>> +
>>> nit: keep it simple, init skel to NULL and use single cleanup goto
>>> label that will destroy skel unconditionally (it deals with NULL just
>>> fine)
>> will do. Thanks.
>>
>>>> + skel->rodata->nr_cpus = nr_cpus;
>>>> +
>>>> + ret = test_map_lookup_percpu_elem__load(skel);
>>>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__load"))
>>>> + goto cleanup;
>>>> +
>>>> ret = test_map_lookup_percpu_elem__attach(skel);
>>>> - ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach");
>>>> + if (!ASSERT_OK(ret, "test_map_lookup_percpu_elem__attach"))
>>>> + goto cleanup;
>>>>
>>>> ret = bpf_map_update_elem(bpf_map__fd(skel->maps.percpu_array_map), &key, buf, 0);
>>>> ASSERT_OK(ret, "percpu_array_map update");
>>> [...]
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-19 7:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 2:24 [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase Feng zhou
2022-05-17 3:09 ` Yonghong Song
2022-05-17 7:05 ` [External] " Feng Zhou
2022-05-19 0:17 ` Andrii Nakryiko
2022-05-19 3:26 ` [External] " Feng Zhou
2022-05-19 4:38 ` Andrii Nakryiko
2022-05-19 7:31 ` Feng Zhou
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).