Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Yonghong Song <yonghong.song@linux.dev>]

On 3/22/24 5:26 AM, Jiri Olsa wrote:
> On Thu, Mar 21, 2024 at 01:01:24PM -0700, Yonghong Song wrote:
>> The current kprobe_multi_bench_attach/kernel test
>> reads sym names from /sys/kernel/tracing/available_filter_functions.
>> Some names do not agree with the corresponding entries in /proc/kallsyms
>> since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
>> Actually, if we pass symbol names in /proc/kallsyms,
>> kprobe_multi_attach will be okay.
>>
>> This patch added a new subtest where addresses are retrieved from
>> /sys/kernel/tracing/available_filter_functions_addrs, and use the
>> address to consule /proc/kallsyms to get the function name.
> hm, I don't understand the reason for this test.. AFAICS test_kprobe_multi_bench_attach
> is doing that already, just reading available_filter_functions file
>
> both available_filter_functions_addrs and available_filter_functions have the
> same functions, there's just extra addresses in available_filter_functions_addrs

The goal is to include those kernel functions filtered in patch 4.
But we cannot use the names from available_filter_functions[_addrs],
and we need to get names from /proc/kallsyms. Hence this patch.
This will test if we give names (<name>.llvm.<hash>) to kernel
for kprobe_multi_attach, things will be okay.

>
>> +	*symsp = syms;
>> +	*cntp = cnt;
>> +
>> +error:
>> +	fclose(f);
>> +	hashmap__free(map);
>> +	if (err) {
>> +		for (i = 0; i < cnt; i++)
>> +			free(syms[i]);
>> +		free(syms);
>> +	}
>> +	return err;
>> +}
>> +
>>   static void test_kprobe_multi_bench_attach(bool kernel)
>>   {
>>   	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
>> @@ -521,6 +617,47 @@ static void test_attach_override(void)
>>   	kprobe_multi_override__destroy(skel);
>>   }
> there's lot of duplicated code in both
>    get_syms_from_addr/get_syms
>    test_attach_kernel_addrs_to_sym/test_kprobe_multi_bench_attach
>
> would be great to put it together

I will give a try in the next revision.

>
>>   
>> +static void test_attach_kernel_addrs_to_sym(void)
>> +{
>> +	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
>> +	struct kprobe_multi_empty *skel;
>> +	struct bpf_link *link;
>> +	char **syms = NULL;
>> +	size_t cnt = 0;
>> +	int i, err;
>> +
>> +	err = get_syms_from_addr(&syms, &cnt);
>> +	if (err == -ENOENT) {
>> +		test__skip();
>> +		return;
>> +	}
>> +	if (!ASSERT_OK(err, "get_syms_from_addr"))
>> +		return;
>> +
>> +	skel = kprobe_multi_empty__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
>> +		goto cleanup;
>> +
>> +	opts.syms = (const char **) syms;
>> +	opts.cnt = cnt;
>> +
>> +	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_empty,
>> +						     NULL, &opts);
>> +
>> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
>> +		goto cleanup;
>> +
>> +	bpf_link__destroy(link);
>> +
>> +cleanup:
>> +	kprobe_multi_empty__destroy(skel);
>> +	if (syms) {
>> +		for (i = 0; i < cnt; i++)
>> +			free(syms[i]);
>> +		free(syms);
>> +	}
>> +}
>> +
>>   void serial_test_kprobe_multi_bench_attach(void)
>>   {
>>   	if (test__start_subtest("kernel"))
>> @@ -550,4 +687,6 @@ void test_kprobe_multi_test(void)
>>   		test_attach_api_fails();
>>   	if (test__start_subtest("attach_override"))
>>   		test_attach_override();
>> +	if (test__start_subtest("kernel_addrs_to_sym"))
>> +		test_attach_kernel_addrs_to_sym();
> we moved the bench subtests to serial_test_kprobe_multi_bench_attach,
> not to clash with others in parallel mode

Okay, I will put it in serial mode in next revision.

>
> jirka