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

[Eduard Zingerman <eddyz87@gmail.com>]

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?

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.
 */

(For some reason it took me a while to understand what happens in this test)

> +	/* tailcall prog + gen_epilogue */
> +	memset(&args, 0, sizeof(args));
> +	prog_fd = bpf_program__fd(skel->progs.syscall_epilogue_tailcall);
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	ASSERT_OK(err, "bpf_prog_test_run_opts");
> +	ASSERT_EQ(args.a, 10001, "args.a");
> +	ASSERT_EQ(topts.retval, 10001 * 2, "topts.retval");
> +
> +done:
> +	epilogue_tailcall__destroy(skel);
> +}

[...]