Re: [PATCH bpf-next v2 2/2] selftests/bpf: Test insns processed breakdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Asserting exact verifier instruction counts in C-based BPF selftests causes test flakiness.
- [Low] Unused test configuration from blindly elevating log levels to 6 without corresponding assertions.
--

commit c4e03a953efa7cd8cf2e1a288a0c55c4f73cace2
Author: Paul Chaignon <paul.chaignon@gmail.com>

selftests/bpf: Test insns processed breakdown

This patch updates global subprog selftests to verify the new verifier
log breakdown of instructions processed by global subprogs. It does this
by increasing log levels and adding new message assertions.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> index 1e08aff7532e..7092a046c018 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c

[ ... ]

> @@ -186,21 +187,24 @@ __weak int raw_tp_u64_array(u64 *ctx __arg_ctx)
>  }
>  
>  SEC("?raw_tp")
> -__success __log_level(2)
> +__success __log_level(6)
> +__msg("insns processed 12+5+2+2")
>  int arg_tag_ctx_raw_tp(void *ctx)

Will hardcoding exact instruction counts cause the tests to become flaky across
different LLVM/Clang versions or optimization levels?

Since test_loader.c natively supports regex matching within __msg using the
{{...}} syntax, would it be more reliable to use a pattern to validate the
log format?

For example, __msg("insns processed {{[0-9]+(\+[0-9]+)*}}") might prevent
failures when the compiler generates slightly different assembly.

[ ... ]

> @@ -321,14 +331,15 @@ int BPF_PROG(arg_tag_ctx_fexit)
>  }
>  
>  SEC("?fmod_ret/" SYS_PREFIX "sys_nanosleep")
> -__success __log_level(2)
> +__success __log_level(6)
> +__msg("insns processed 7+2+2")
>  int BPF_PROG(arg_tag_ctx_fmod_ret)
>  {
>  	return tracing_subprog_void(ctx) + tracing_subprog_u64(ctx);
>  }
>  
>  SEC("?lsm/bpf")
> -__success __log_level(2)
> +__success __log_level(6)
>  int BPF_PROG(arg_tag_ctx_lsm)
>  {
>  	int ret;

Is the log level increase to 6 intentional for arg_tag_ctx_lsm and other
similar tests in this patch (like anon_user_mem_valid and arg_tag_dynptr)?

These tests don't appear to have corresponding __msg assertions added to
actually verify the resulting stats breakdown, which might just add
unnecessary overhead and log data to the test execution.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5c3514dbc879f22ba3f9eff8a2c730e13bd0572c.1777398528.git.paul.chaignon@gmail.com?part=2