Re: [PATCH bpf-next] selftests/bpf: Add kptr-xchg benchmark

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index d3655a706482..76782b442aa9 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> diff --git a/tools/testing/selftests/bpf/benchs/bench_kptr_xchg.c b/tools/testing/selftests/bpf/benchs/bench_kptr_xchg.c
> new file mode 100644
> index 000000000000..b8a0d346fda6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/benchs/bench_kptr_xchg.c
> @@ -0,0 +1,96 @@

[ ... ]

> +enum {
> +	ARG_NR_LOOPS = 7000,
> +};
> +
> +static const struct argp_option opts[] = {
> +	{ "nr_loops", ARG_NR_LOOPS, "nr_loops", 0,
> +	  "Set number of bpf_kptr_xchg() calls per trigger"},
> +	{},
> +};
> +
> +static error_t parse_arg(int key, char *arg, struct argp_state *state)
> +{
> +	switch (key) {
> +	case ARG_NR_LOOPS:
> +		args.nr_loops = strtol(arg, NULL, 10);
> +		break;

Should parse_arg() validate the nr_loops value here? Looking at the BPF
program, the loop is capped at MAX_XCHG_LOOPS (4096):

tools/testing/selftests/bpf/progs/kptr_xchg_bench.c:benchmark() {
	for (i = 0; i < MAX_XCHG_LOOPS; i++) {
		if (i >= nr_loops)
			break;
		...
	}
}

So passing --nr_loops 100000 silently benchmarks only 4096 iterations,
and --nr_loops -1 (which strtol accepts and wraps to 4294967295 when
assigned to __u32) also silently runs 4096. The user gets throughput
numbers for a different workload than requested.

bench_trigger.c already handles this by rejecting out-of-range values at
parse time:

	ret = strtol(arg, NULL, 10);
	if (ret < 1 || ret > MAX_TRIG_BATCH_ITERS) {
		fprintf(stderr, "invalid --trig-batch-iters value ...\n");
		argp_usage(state);
	}

Could bench_kptr_xchg.c add the same range check against MAX_XCHG_LOOPS
to make the cap visible rather than silent?

> +static const struct argp bench_kptr_xchg_argp = {
> +	.options = opts,
> +	.parser = parse_arg,
> +};

Is bench_kptr_xchg_argp intended to be static? All other benches export
their argp non-static (bench_bpf_loop_argp, bench_xdp_lb_argp,
bench_sockmap_argp, etc.) and register them in two places:

1. bench.c declares extern struct argp bench_*_argp
2. bench.c adds them to the static bench_parsers[] child list

This patch adds neither. Consequences:

`./bench --help` never shows the "kptr-xchg" option group or --nr_loops,
because --help is served from the first-pass child list.

--nr_loops only works today because bench_bpf_loop_argp and
bench_bpf_for_argp (both already in bench_parsers[]) also declare a
long option named "nr_loops", so the first pass consumes the argument
into an unrelated bench's args struct rather than erroring out. The
second pass then parses it again with the correct parser, so the
documented invocation happens to work. Any option name unique to this
bench would make the first pass abort with "unrecognized option".

Should bench_kptr_xchg_argp drop static, get an extern declaration in
bench.c, and be added to bench_parsers[] alongside the other benches?

> diff --git a/tools/testing/selftests/bpf/progs/kptr_xchg_bench.c b/tools/testing/selftests/bpf/progs/kptr_xchg_bench.c
> new file mode 100644
> index 000000000000..363883073e2c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kptr_xchg_bench.c

[ ... ]

> +SEC("fentry/" SYS_PREFIX "sys_getpgid")
> +int benchmark(void *ctx)
> +{
> +	struct bin_data *old;
> +	u32 i;
> +
> +	for (i = 0; i < MAX_XCHG_LOOPS; i++) {
> +		if (i >= nr_loops)
> +			break;
> +
> +		old = bpf_kptr_xchg(&ptr, NULL);
> +		if (old)
> +			bpf_obj_drop(old);
> +	}
> +
> +	__sync_add_and_fetch(&hits, i);
> +	return 0;
> +}
> +
> +/*
> + * BTF FUNC records are not generated for kfuncs referenced only through
> + * optimized paths. Keep bpf_obj_drop() visible to libbpf's kfunc linker.
> + */
> +void __btf_root(void)
> +{
> +	bpf_obj_drop(NULL);
> +}

Is __btf_root() needed in this file? The workaround was copied from
kptr_xchg_inline.c, but that file actually requires it because the only
reference to bpf_obj_drop is inside a __naked function's inline assembly
via __imm(bpf_obj_drop), so clang never sees a C-level call and emits no
BTF FUNC record. That file's own comment says exactly that: "BTF FUNC
records are not generated for kfuncs referenced from inline assembly."

kptr_xchg_bench.c has no inline assembly. benchmark() calls the kfunc
with a plain C call:

	old = bpf_kptr_xchg(&ptr, NULL);
	if (old)
		bpf_obj_drop(old);

clang cannot prove old is always NULL (bpf_kptr_xchg is an opaque helper
call), so the call site survives -O2 and a BTF FUNC record for
bpf_obj_drop is emitted normally. This is confirmed by twenty other
programs in progs/ that call bpf_obj_drop() from ordinary C and define
no such helper: linked_list.c, refcounted_kptr.c, irq.c,
local_kptr_stash.c, rbtree_search.c, bpf_qdisc_fifo.c, etc.
kptr_xchg_inline.c is the only file in the tree that actually needs the
hack.

The rewritten comment also generalises the original's precise claim
("from inline assembly") into "referenced only through optimized paths",
which is not a real libbpf/clang behaviour and does not describe
anything happening in this file.

The function is never loaded (it is unreferenced, non-SEC() .text, so
libbpf never appends it as a subprog, which is why the bpf_obj_drop(NULL)
inside it never reaches the verifier's KF_RELEASE check), so there is no
load failure. But it is dead code carrying an incorrect technical
rationale that invites copying into future programs.

Should both the function and the comment be removed?


---
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/31362294949