Re: [PATCH v4 1/4] kallsyms: Add test_kallsyms_perf module to benchmark lookup latency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] test_kallsyms: infinite loop on large num_iters due to integer overflow
- [Medium] test_kallsyms: benchmark latency includes preemption sleep time
--

commit 3ac6ce68ed3764cca0a42a3e09873db349d331f2
Author: Jim Cromie <jim.cromie@gmail.com>

kallsyms: Add test_kallsyms_perf module to benchmark lookup latency

This commit adds a lightweight microbenchmark module to evaluate optimizations
and measure performance regressions across kallsyms lookups. It benchmarks
name-to-address, address-to-name, and sequential table scans.

> diff --git a/lib/test_kallsyms_perf.c b/lib/test_kallsyms_perf.c
> new file mode 100644
> index 0000000000000..03ff5f1d51c5d
> --- /dev/null
> +++ b/lib/test_kallsyms_perf.c
[ ... ]
> +static void run_name_lookup_bench(unsigned int iters)
> +{
[ ... ]
> +	/* 1. Name search: Existing symbols (Hits) */
> +	for (i = 0; i < iters; i += BENCH_CHUNK_SIZE) {

[Severity: High]
Does this unconditionally add BENCH_CHUNK_SIZE to i in a way that can wrap
around?

If a user sets num_iters via sysfs to a value approaching UINT_MAX
(for example, UINT_MAX - 100), could i wrap around to a small number?

If so, will the termination condition i < iters never be met, causing an
infinite loop that permanently holds bench_lock?

> +		unsigned int chunk = min_t(unsigned int, BENCH_CHUNK_SIZE, iters - i);
> +		unsigned int j;
> +
> +		cond_resched();
> +		t0 = ktime_get_ns();
> +		for (j = 0; j < chunk; j++) {
> +			const char *sym = hit_symbols[(i + j) % nr_hits];
> +
> +			kallsyms_on_each_match_symbol(match_cb, sym, &addr);

[Severity: Medium]
Do these kallsyms traversal APIs inherently invoke cond_resched() within their
internal iteration loops?

If so, does calling them inside the timing bracket (between t0 and t1) defeat
the chunking mitigation? Could this include preemption sleep time in the
measured benchmark latency?

> +			OPTIMIZER_HIDE_VAR(addr);
> +		}
> +		t1 = ktime_get_ns();
> +		dt_hit += t1 - t0;
> +	}
> +
> +	/* 2. Name search: Non-existent symbols (Misses - 17 bsearch probes) */
> +	for (i = 0; i < iters; i += BENCH_CHUNK_SIZE) {

[Severity: High]
Can this loop also suffer from the same integer overflow if num_iters is very
large?

> +		unsigned int chunk = min_t(unsigned int, BENCH_CHUNK_SIZE, iters - i);
> +		unsigned int j;
> +
> +		cond_resched();
> +		t0 = ktime_get_ns();
> +		for (j = 0; j < chunk; j++) {
> +			const char *sym = miss_symbols[(i + j) % nr_misses];
> +
> +			kallsyms_on_each_match_symbol(match_cb, sym, &addr);
> +			OPTIMIZER_HIDE_VAR(addr);
> +		}
> +		t1 = ktime_get_ns();
> +		dt_miss += t1 - t0;
> +	}
[ ... ]
> +static void run_address_lookup_bench(unsigned int iters)
> +{
[ ... ]
> +	/* 1. Address-to-name resolution (sprint_symbol) */
> +	for (i = 0; i < iters; i += BENCH_CHUNK_SIZE) {

[Severity: High]
Is there a similar risk of wrap-around in this loop when num_iters approaches
UINT_MAX?

> +		unsigned int chunk = min_t(unsigned int, BENCH_CHUNK_SIZE, iters - i);
> +		unsigned int j;
> +
> +		cond_resched();
> +		t0 = ktime_get_ns();
> +		for (j = 0; j < chunk; j++) {
> +			unsigned long addr = addrs[(i + j) % nr_addrs];
> +
> +			sprint_symbol(symname, addr);
> +			barrier_data(symname);
> +		}
> +		t1 = ktime_get_ns();
> +		dt_sprint += t1 - t0;
> +	}
> +
> +	/* 2. Address without offset (sprint_symbol_no_offset) */
> +	for (i = 0; i < iters; i += BENCH_CHUNK_SIZE) {

[Severity: High]
Could this loop also become infinite due to the same integer overflow
condition?

> +		unsigned int chunk = min_t(unsigned int, BENCH_CHUNK_SIZE, iters - i);
> +		unsigned int j;
> +
> +		cond_resched();
> +		t0 = ktime_get_ns();
> +		for (j = 0; j < chunk; j++) {
> +			unsigned long addr = addrs[(i + j) % nr_addrs];
> +
> +			sprint_symbol_no_offset(symname, addr);
> +			barrier_data(symname);
> +		}
> +		t1 = ktime_get_ns();
> +		dt_bldid += t1 - t0;
> +	}
[ ... ]
> +static void run_table_walk_bench(void)
> +{
[ ... ]
> +	for (i = 0; i < iter; i++) {
> +		total_symbols = 0;
> +		cond_resched();
> +		t0 = ktime_get_ns();
> +		kallsyms_on_each_symbol(count_cb, &total_symbols);

[Severity: Medium]
Does kallsyms_on_each_symbol() also natively invoke cond_resched() inside its
iteration loop?

If so, could placing this call inside the timing bracket include preemption
sleep time and inflate the measured walk latency?

> +		t1 = ktime_get_ns();
> +		dt_walk += t1 - t0;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-ksyms-tune-v4-0-92acea84b911@gmail.com?part=1