Re: [PATCH bpf-next] selftest/bpf: make pyperf600 a success again

>     pyperf600 has been running into 8K BPF_COMPLEXITY_LIMIT_JMP_SEQ limitations
>     for a long while now, after some internal compiler changes.
>
>     Until BPF verifier is bestowed with scalar evolution logic, make that test
>     actually work by doing what would anyone should do in such situations: by
>     moving repeatable per-iteration work into independently verified global
>     functions.
>
>     `void *` argument is a problem for global funcs, but a static function wrapper
>     doing necessary casts and a bit of __arg_nonnull magic dust is all it takes.
>
>     Signed-off-by: Andrii Nakryiko <[email protected]>

This isn't a bug, but could this sentence be tightened? The phrase "by
doing what would anyone should do in such situations: by moving ..."
mixes "what would anyone do" with "what anyone should do" and repeats
the "by" clause on both sides of the colon. Also, the changelog says
"global functions" (plural) while the diff adds exactly one global
function (__get_frame_data).

> diff --git a/tools/testing/selftests/bpf/progs/pyperf.h b/tools/testing/selftests/bpf/progs/pyperf.h
> index 86484f07e1d19..fd93a96e59015 100644
> --- a/tools/testing/selftests/bpf/progs/pyperf.h
> +++ b/tools/testing/selftests/bpf/progs/pyperf.h
> @@ -85,9 +85,11 @@ static void *get_thread_state(void *tls_base, PidData *pidData)
>  	return thread_state;
>  }
>
> -static __always_inline bool get_frame_data(void *frame_ptr, PidData *pidData,
> -					   FrameData *frame, Symbol *symbol)
> +__weak bool __get_frame_data(long frame_ptr_, PidData *pidData __arg_nonnull,
> +			     FrameData *frame __arg_nonnull, Symbol *symbol __arg_nonnull)
>  {
> +	void *frame_ptr = (void *)frame_ptr_;
> +
>  	// read data from PyFrameObject
>  	bpf_probe_read_user(&frame->f_back,
>  			    sizeof(frame->f_back),

[ ... ]

> @@ -119,6 +121,12 @@ static __always_inline bool get_frame_data(void *frame_ptr, PidData *pidData,
>  	return true;
>  }
>
> +static __always_inline bool get_frame_data(void *frame_ptr, PidData *pidData,
> +					   FrameData *frame, Symbol *symbol)
> +{
> +	return __get_frame_data((long)frame_ptr, pidData, frame, symbol);
> +}
> +
>  struct {
>  	__uint(type, BPF_MAP_TYPE_HASH);
>  	__uint(max_entries, 1);

This isn't a bug, but was the intent to change all nine pyperf variants?
pyperf.h already carries a per-variant inlining switch used twice in the
same file (the #ifdef SUBPROGS block at get_thread_state and the #ifdef
GLOBAL_FUNC / #elif SUBPROGS block at __on_event). The new __get_frame_data
is unconditionally __weak, so all nine includers change shape at once,
including pyperf_subprogs.c and pyperf_global.c whose reason for existing
is to exercise the static-noinline-subprog and global-func paths
specifically, and pyperf50/100/180 which exercise full unrolling.

The descriptions in prog_tests/bpf_verif_scale.c that document the
intended shape of these objects (for pyperf600: "Asm loop count -> 4. 16k
insns in loop body. Total program size ~82k insns.") are no longer
accurate for the smaller loop body and are not updated here. The changelog
only motivates pyperf600.

Would gating the new global function the way get_thread_state()/
__on_event() are already gated keep the subprogs/global/unrolled variants
testing what their names suggest? If it is meant to apply everywhere,
would it be worth refreshing the size comments in bpf_verif_scale.c?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31754735233