Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for updating attached freplace prog to prog_array map

[Leon Hwang <leon.hwang@linux.dev>]

On 2024/7/27 03:38, Yonghong Song wrote:
> 
> On 7/26/24 8:39 AM, Leon Hwang wrote:
>> Add a selftest to confirm the issue, which gets -EINVAL when update
>> attached freplace prog to prog_array map, has been fixed.
>>
>> cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
>> 327/25  tailcalls/tailcall_freplace:OK
>> 327     tailcalls:OK
>> Summary: 1/25 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> 
> LGTM with some comments below.
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> 
>> ---
>>   .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
>>   .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
>>   .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
>>   3 files changed, 110 insertions(+), 1 deletion(-)
>>   create mode 100644
>> tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>

[...]

>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> new file mode 100644
>> index 0000000000000..2966efc06ae8f
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +struct {
>> +    __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>> +    __uint(max_entries, 1);
>> +    __uint(key_size, sizeof(__u32));
>> +    __uint(value_size, sizeof(__u32));
>> +} jmp_table SEC(".maps");
>> +
>> +int count = 0;
>> +
>> +SEC("freplace")
>> +int entry_freplace(struct __sk_buff *skb)
>> +{
>> +    count++;
>> +
> remove empty line here.
>> +    bpf_tail_call_static(skb, &jmp_table, 0);
>> +
> remove empty line here.
>> +    return count;
>> +}
>> +
>> +char __license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> new file mode 100644
>> index 0000000000000..980bb810b481c
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +__noinline
>> +int subprog(struct __sk_buff *skb)
>> +{
>> +    volatile int ret = 1;
>> +
> remove empty line here.

Should we remove this empty line?

./scripts/checkpatch.pl:

WARNING: Missing a blank line after declarations
#158: FILE: tools/testing/selftests/bpf/progs/tc_bpf2bpf.c:11:
+	int ret = 1;
+	__sink(ret);

>> +    asm volatile (""::"r+"(ret));
> remove above 'volatile' key word and replace asm volatile with __sink(ret).
>> +    return ret;
>> +}
>> +
>> +SEC("tc")
>> +int entry_tc(struct __sk_buff *skb)
>> +{
>> +    return subprog(skb);
>> +}
>> +
>> +char __license[] SEC("license") = "GPL";