Re: [PATCH 2/8] xor: improve the runtime selection benchmark

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing clock-tick synchronization with a fixed number of loop iterations causes zero-duration time deltas on coarse clocks, leading to artificial throughput inflation, integer overflow, and random algorithm selection.
--

--- Patch [2]: [PATCH 2/8] xor: improve the runtime selection benchmark ---
commit 2205984bf337f5c013e14971e819bb6346e19853
Author: Christoph Hellwig <[email protected]>

xor: improve the runtime selection benchmark

This commit modifies the XOR block benchmark to use ktime_get_ns for timing
and switches to 4 + 1 disks for a more realistic load. It also reports the
throughput on the data disks instead of the parity disk.

> diff --git a/lib/raid/xor/xor-core.c b/lib/raid/xor/xor-core.c
> index 50931fbf0324e..f9d61b096b117 100644
> --- a/lib/raid/xor/xor-core.c
> +++ b/lib/raid/xor/xor-core.c

[ ... ]

> -#define BENCH_SIZE	4096
> +#define BENCH_SIZE	SZ_4K
> +#define NR_SRCS		4
>  #define REPS		800U
>  
> -static void __init
> -do_xor_speed(struct xor_block_template *tmpl, void *b1, void *b2)
> +static void __init do_xor_speed(struct xor_block_template *tmpl, void *dest,
> +		void *srcs[NR_SRCS])
>  {
> -	int speed;
> -	unsigned long reps;
> -	ktime_t min, start, t0;
> -	void *srcs[1] = { b2 };
> +	u64 t;
> +	int i;
>  
>  	preempt_disable();
> -
> -	reps = 0;
> -	t0 = ktime_get();
> -	/* delay start until time has advanced */
> -	while ((start = ktime_get()) == t0)
> -		cpu_relax();
> -	do {
> +	t = ktime_get_ns();
> +	for (i = 0; i < REPS; i++) {
>  		mb(); /* prevent loop optimization */
> -		tmpl->xor_gen(b1, srcs, 1, BENCH_SIZE);
> +		tmpl->xor_gen(dest, srcs, NR_SRCS, BENCH_SIZE);
>  		mb();
> -	} while (reps++ < REPS || (t0 = ktime_get()) == start);
> -	min = ktime_sub(t0, start);
> -
> +	}
> +	t = max(ktime_get_ns() - t, 1);

[Severity: High]
Does removing the clock-tick synchronization create an issue with low
resolution clocks?

If a high-resolution clocksource is not yet available during early boot,
the 800 loop iterations might complete within a single clock tick. If that
happens, the elapsed time t evaluates to 0 and gets clamped to 1.

>  	preempt_enable();
>  
> -	// bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s]
> -	speed = (1000 * reps * BENCH_SIZE) / (unsigned int)ktime_to_ns(min);
> -	tmpl->speed = speed;
> +	/* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
> +	tmpl->speed = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);

[Severity: High]
Can this division silently overflow the 32-bit signed integer tmpl->speed?

If t is clamped to 1 ns, it yields a quotient of approximately 13.1 billion.
This overflows the 32-bit integer and could result in negative or
astronomically high throughput values. 

This might break the calibration logic and cause the kernel to select a
suboptimal XOR algorithm.

>  
> -	pr_info("   %-16s: %5d MB/sec\n", tmpl->name, speed);
> +	pr_info("   %-16s: %5d MB/sec\n", tmpl->name, tmpl->speed);
>  }

[ ... ]

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