Re: [PATCH 5/6] selftests/bpf: libarena: Add a benchmark for malloc()/calloc()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The benchmark incorrectly reports the 'allocations per second' metric as 'invocations', and includes an unmatched closing parenthesis.
- [Medium] Missing bounds check causes silent 32-bit integer truncation for the `--nallocs` argument.
- [Low] An unsigned validation value is formatted with a signed `%ld` specifier, causing it to print negative numbers on error.
--

--- Patch [5]: [PATCH 5/6] selftests/bpf: libarena: Add a benchmark for malloc()/calloc() ---
commit cfe60c8125bcfe8399db973cf6b5a739e50079d3
Author: Emil Tsalapatis <[email protected]>

selftests/bpf: libarena: Add a benchmark for malloc()/calloc()
    
Add a benchmark for measuring the performance of the malloc()/calloc()
arena allocator calls. This is useful as a basic allocator performance
check that we can easily expand later. As with the regular arena
allocator, focus on sub-page allocations that cannot be satisfied
efficiently with the BPF arena page allocation code.

> diff --git a/tools/testing/selftests/bpf/benchs/bench_libarena.c b/tools/testing/selftests/bpf/benchs/bench_libarena.c
> new file mode 100644
> index 0000000000000..24e432244bf5d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/benchs/bench_libarena.c
> @@ -0,0 +1,210 @@

[ ... ]

> +static error_t parse_arg(int key, char *arg, struct argp_state *state)
> +{
> +	unsigned long value;
> +
> +	switch (key) {
> +	case ARG_LIBARENA_ALLOC_SIZE:
> +		value = strtoull(arg, NULL, 10);
> +		if (!value || value >= UINT_MAX) {
> +			fprintf(stderr, "invalid alloc_size: %ld", value);

[Severity: Low]
Does this print a negative number on overflow since value is an unsigned long
and the format specifier is %ld instead of %lu?

> +			argp_usage(state);
> +		}
> +		args.alloc_size = value;
> +		break;
> +	case ARG_LIBARENA_NALLOCS:
> +		args.nallocs = strtoull(arg, NULL, 10);

[Severity: Medium]
Since args.nallocs is parsed as a 64-bit value without upper bounds validation,
and ctx.skel->bss->bench_nallocs is a 32-bit u32, does this result in silent
32-bit integer truncation when copying it later in setup_common()?

> +		break;
> +	default:
> +		return ARGP_ERR_UNKNOWN;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void report_final(struct bench_res res[], int res_cnt)
> +{
> +	unsigned long duration_ns = 0;
> +	long hits = 0;
> +	int i;
> +
> +	for (i = 0; i < res_cnt; i++) {
> +		duration_ns += res[i].duration_ns;
> +		hits += res[i].hits;
> +	}
> +
> +	if (!hits || !res_cnt) {
> +		printf("Summary: no runs measured\n");
> +		return;
> +	}
> +
> +	printf("Summary: %.3lf ns/op, %.0lf invocations for %u allocations/invocation)\n",
> +	       duration_ns / (double)hits, hits / (double)res_cnt,
> +	       ctx.skel->bss->bench_nallocs);

[Severity: Medium]
Since hits tracks the total number of allocations, does hits / res_cnt
incorrectly report the allocations metric as invocations?

Also, there is an unmatched closing parenthesis at the end of the format
string.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5