Re: [PATCH bpf-next v7 9/9] selftests/bpf: Add struct_ops prog private stack tests

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

On 10/30/24 4:29 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Oct 29, 2024 at 03:17:23PM -0700, Yonghong Song wrote:
>> The third test is the same callback function recursing itself. At run time,
>> the jit trampoline recursion check kicks in to prevent the recursion.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  94 ++++++++++++++++
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   5 +
>>   .../bpf/prog_tests/struct_ops_private_stack.c | 106 ++++++++++++++++++
>>   .../bpf/progs/struct_ops_private_stack.c      |  62 ++++++++++
>>   .../bpf/progs/struct_ops_private_stack_fail.c |  62 ++++++++++
>>   .../progs/struct_ops_private_stack_recur.c    |  50 +++++++++
>>   6 files changed, 379 insertions(+)
>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_private_stack.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_private_stack.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_private_stack_fail.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_private_stack_recur.c
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index 8835761d9a12..eb761645551a 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> ...
>> +__bpf_kfunc void bpf_testmod_ops3_call_test_1(void)
>> +{
>> +	st_ops3->test_1();
>> +}
> ...
>> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_private_stack.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_private_stack.c
>> new file mode 100644
>> index 000000000000..4006879ca3fe
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_private_stack.c
> ...
>> +static void test_private_stack_recur(void)
>> +{
>> +	struct struct_ops_private_stack_recur *skel;
>> +	struct bpf_link *link;
>> +	int err;
>> +
>> +	skel = struct_ops_private_stack_recur__open();
>> +	if (!ASSERT_OK_PTR(skel, "struct_ops_private_stack_recur__open"))
>> +		return;
>> +
>> +	if (skel->data->skip) {
>> +		test__skip();
>> +		goto cleanup;
>> +	}
>> +
>> +	err = struct_ops_private_stack_recur__load(skel);
>> +	if (!ASSERT_OK(err, "struct_ops_private_stack_recur__load"))
>> +		goto cleanup;
>> +
>> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
>> +	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
>> +		goto cleanup;
>> +
>> +	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
>> +
>> +	ASSERT_EQ(skel->bss->val_j, 3, "val_j");
>> +
>> +	bpf_link__destroy(link);
>> +
>> +cleanup:
>> +	struct_ops_private_stack_recur__destroy(skel);
>> +}
> ...
>> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_private_stack_recur.c b/tools/testing/selftests/bpf/progs/struct_ops_private_stack_recur.c
>> new file mode 100644
>> index 000000000000..15d4e914dc92
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_private_stack_recur.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +#if defined(__TARGET_ARCH_x86)
>> +bool skip __attribute((__section__(".data"))) = false;
>> +#else
>> +bool skip = true;
>> +#endif
>> +
>> +void bpf_testmod_ops3_call_test_1(void) __ksym;
>> +
>> +int val_i, val_j;
>> +
>> +__noinline static int subprog2(int *a, int *b)
>> +{
>> +	return val_i + a[10] + b[20];
>> +}
>> +
>> +__noinline static int subprog1(int *a)
>> +{
>> +	/* stack size 400 bytes */
>> +	int b[100] = {};
>> +
>> +	b[20] = 2;
>> +	return subprog2(a, b);
>> +}
>> +
>> +
>> +SEC("struct_ops")
>> +int BPF_PROG(test_1)
>> +{
>> +	/* stack size 400 bytes */
>> +	int a[100] = {};
>> +
>> +	a[10] = 1;
>> +	val_j += subprog1(a);
>> +	bpf_testmod_ops3_call_test_1();
>> +	return 0;
>> +}
>> +
>> +SEC(".struct_ops")
>> +struct bpf_testmod_ops3 testmod_1 = {
>> +	.test_1 = (void *)test_1,
>> +};
> This is delta, and, while this shouldn't happen for SCX, it'd be great if
> SCX can tell BPF to call a function when recursion check triggers and
> ignores a call, so that SCX can trigger error, report it and eject the
> scheduler.

I had an offline discussion with Tejun. A callback function like
   prog->aux->recursion_skipped(prog)
will be provided and if not null the function will be called whenever
a recursion skip happens.

The subsystem requires to define recursion_skipped
callback function and assign it to prog->aux->recursion_skipped if necessary,
if it want to get some error information back.

>
> Thanks.
>