| Message ID | 20220516022453.68420-1-zhoufeng.zf@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase | expand |
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; > +} > + [...]
在 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; >> +} >> + > [...]
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"); [...]
在 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"); > [...]
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"); > > [...] > >
在 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"); >>> [...] >>
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; }