Re: [PATCH v4 bpf-next 7/9] selftests/bpf: Add tailcall epilogue test

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/28/24 11:16 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This patch adds a gen_epilogue test to test a main prog
>> using a bpf_tail_call.
>>
>> A non test_loader test is used. The tailcall target program,
>> "test_epilogue_subprog", needs to be used in a struct_ops map
>> before it can be loaded. Another struct_ops map is also needed
>> to host the actual "test_epilogue_tailcall" struct_ops program
>> that does the bpf_tail_call. The earlier test_loader patch
>> will attach all struct_ops maps but the bpf_testmod.c does
>> not support >1 attached struct_ops.
>>
>> The earlier patch used the test_loader which has already covered
>> checking for the patched pro/epilogue instructions. This is done
>> by the __xlated tag.
>>
>> This patch goes for the regular skel load and syscall test to do
>> the tailcall test that can also allow to directly pass the
>> the "struct st_ops_args *args" as ctx_in to the
>> SEC("syscall") program.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> +static void test_tailcall(void)
>> +{
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
>> +	struct epilogue_tailcall *skel;
>> +	struct st_ops_args args;
>> +	int err, prog_fd;
>> +
>> +	skel = epilogue_tailcall__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "epilogue_tailcall__open_and_load"))
>> +		return;
>> +
>> +	topts.ctx_in = &args;
>> +	topts.ctx_size_in = sizeof(args);
>> +
>> +	skel->links.epilogue_tailcall =
>> +		bpf_map__attach_struct_ops(skel->maps.epilogue_tailcall);
>> +	if (!ASSERT_OK_PTR(skel->links.epilogue_tailcall, "attach_struct_ops"))
>> +		goto done;
>> +
> 
> Nitpick:
> Both test_epilogue_tailcall and test_epilogue_subprog would be
> augmented with epilogue, and we know that tail call run as expected
> because only test_epilogue_subprog does +1, right?

Yes. and also the epilogue of the test_epilogue_subprog is executed.

> 
> If above is true, could you please update the comment a bit, e.g.:
> 
> /* Both test_epilogue_tailcall and test_epilogue_subprog are
>   * augmented with epilogue. When syscall_epilogue_tailcall()
>   * is run test_epilogue_tailcall() is triggered,
>   * it executes a tail call and control is transferred to
>   * test_epilogue_subprog(). Only test_epilogue_subprog()
>   * does args->a += 1, thus final args.a value of 10001
>   * guarantees that tail call was executed as expected.
>   */

Added. I massaged the wordings a little.