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

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

On 7/26/24 8:28 PM, Leon Hwang wrote:
>
> 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);
sorry, we should keep blank line between 'int ret = 1' and '__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";