From: Feng Zhou <zhoufeng.zf@bytedance.com>
To: Yonghong Song <yhs@fb.com>,
ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
kafai@fb.com, songliubraving@fb.com, john.fastabend@gmail.com,
kpsingh@kernel.org, rostedt@goodmis.org, mingo@redhat.com,
jolsa@kernel.org, davemarchevsky@fb.com, joannekoong@fb.com,
geliang.tang@suse.com
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org, duanxiongchun@bytedance.com,
songmuchun@bytedance.com, wangdongdong.6@bytedance.com,
cong.wang@bytedance.com, zhouchengming@bytedance.com,
yosryahmed@google.com
Subject: Re: [External] Re: [PATCH bpf-next] selftests/bpf: fix some bugs in map_lookup_percpu_elem testcase
Date: Tue, 17 May 2022 15:05:42 +0800 [thread overview]
Message-ID: <44a30595-6a50-6ded-5ecc-18fd1e56abda@bytedance.com> (raw)
In-Reply-To: <80ab09cf-6072-a75a-082d-2721f6f907ef@fb.com>
在 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;
>> +}
>> +
> [...]
next prev parent reply other threads:[~2022-05-17 7:06 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Feng Zhou [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=44a30595-6a50-6ded-5ecc-18fd1e56abda@bytedance.com \
--to=zhoufeng.zf@bytedance.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=daniel@iogearbox.net \
--cc=davemarchevsky@fb.com \
--cc=duanxiongchun@bytedance.com \
--cc=geliang.tang@suse.com \
--cc=joannekoong@fb.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=songliubraving@fb.com \
--cc=songmuchun@bytedance.com \
--cc=wangdongdong.6@bytedance.com \
--cc=yhs@fb.com \
--cc=yosryahmed@google.com \
--cc=zhouchengming@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).