Re: [PATCH 7/8] raid6: improve the runtime selection benchmark

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Potential division by zero kernel panic when calculating benchmark throughput if the elapsed time `t` evaluates to 0.
- [Medium] The reported throughput for the XOR algorithm is artificially inflated by a factor of 2.
- [Low] The benchmark inner loop variable `int i` redundantly shadows the outer function-scoped variable `unsigned int i`.
--

--- Patch [7]: [PATCH 7/8] raid6: improve the runtime selection benchmark ---
commit ae782577f509b50023ef73d6d96f6fe2f366a500
Author: Christoph Hellwig <[email protected]>

raid6: improve the runtime selection benchmark
    
Use plain ktime_get_ns for the timing, use 8 + 2 disks for a realistic
load, and report the throughput on the data disks as that is what
storage systems are measured on.

> diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
> index e7984bde5157f..c44b986e319e3 100644
> --- a/lib/raid/raid6/algos.c
> +++ b/lib/raid/raid6/algos.c
[ ... ]
> @@ -152,40 +152,39 @@ void raid6_recov_datap(int disks, size_t bytes, int faila, void **ptrs)
>  }
>  EXPORT_SYMBOL_GPL(raid6_recov_datap);
>  
> -#define RAID6_TIME_JIFFIES_LG2	4
> -#define RAID6_TEST_DISKS	8
> +#define BENCH_SIZE	SZ_4K
> +#define NR_SRCS		8
> +#define NR_DISKS	(NR_SRCS + 2)
> +#define REPS		800U
>  
> -static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
> -		const int disks)
> +static int raid6_choose_gen(void *dptrs[NR_DISKS], const int disks)
>  {
>  	/* work on the second half of the disks */
> -	int start = (disks >> 1) - 1, stop = disks - 3;
> +	int start = (disks / 2) - 1, stop = disks - 3;
>  	const struct raid6_calls *best = NULL;
>  	unsigned long bestgenperf = 0;
>  	unsigned int i;
>  
>  	for (i = 0; i < raid6_nr_algos; i++) {
>  		const struct raid6_calls *algo = raid6_algos[i];
> -		unsigned long perf = 0, j0, j1;
> +		unsigned long perf = 0;
> +		u64 t;
> +		int i;

[Severity: Low]
This isn't a bug, but `int i` declared here shadows the function-scoped
`unsigned int i` used for the outer loop.

>  
>  		preempt_disable();
> -		j0 = jiffies;
> -		while ((j1 = jiffies) == j0)
> -			cpu_relax();
> -		while (time_before(jiffies,
> -				    j1 + (1<<RAID6_TIME_JIFFIES_LG2))) {
> -			algo->gen_syndrome(disks, PAGE_SIZE, *dptrs);
> -			perf++;
> -		}
> +		t = ktime_get_ns();
> +		for (i = 0; i < REPS; i++)
> +			algo->gen_syndrome(disks, BENCH_SIZE, dptrs);
> +		t = ktime_get_ns() - t;
>  		preempt_enable();
>  
> +		/* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
> +		perf = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);

[Severity: High]
Is it possible for `t` to be 0 here, leading to a division by zero panic?

If the system uses a coarse clock source (like jiffies during early boot before
high-resolution timers are initialized) and the benchmark iterations complete
within the same timer tick, `t` might evaluate to 0.

[ ... ]
> @@ -197,28 +196,22 @@ static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
>  	static_call_update(raid6_xor_syndrome_impl, best->xor_syndrome);
>  
>  	pr_info("raid6: using algorithm %s gen() %ld MB/s\n",
> -		best->name,
> -		(bestgenperf * HZ * (disks - 2)) >>
> -		(20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2));
> +		best->name, bestgenperf);
>  
>  	if (best->xor_syndrome) {
> -		unsigned long perf = 0, j0, j1;
> +		u64 t;
> +		int i;

[Severity: Low]
This isn't a bug, but `int i` declared here also shadows the function-scoped
`unsigned int i`.

>  
>  		preempt_disable();
> -		j0 = jiffies;
> -		while ((j1 = jiffies) == j0)
> -			cpu_relax();
> -		while (time_before(jiffies,
> -				   j1 + (1 << RAID6_TIME_JIFFIES_LG2))) {
> -			best->xor_syndrome(disks, start, stop,
> -					   PAGE_SIZE, *dptrs);
> -			perf++;
> -		}
> +		t = ktime_get_ns();
> +		for (i = 0; i < REPS; i++)
> +			best->xor_syndrome(disks, start, stop, BENCH_SIZE,
> +				dptrs);
> +		t = ktime_get_ns() - t;
>  		preempt_enable();
>  
> -		pr_info("raid6: .... xor() %ld MB/s, rmw enabled\n",
> -			(perf * HZ * (disks - 2)) >>
> -			(20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2 + 1));
> +		pr_info("raid6: .... xor() %llu MB/s, rmw enabled\n",
> +			div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t));

[Severity: Medium]
Does this calculation artificially inflate the reported throughput for the XOR
algorithm by a factor of 2?

The `xor_syndrome` benchmark operates on a subset of disks from `start` to
`stop` (which is `(disks / 2) - 1` through `disks - 3`). For the default
`NR_DISKS` (10), this processes 4 data disks. However, the throughput
calculation multiplies by `NR_SRCS` (8) instead of the actual number of disks
processed. The previous implementation correctly adjusted for this using an
extra bit shift (`+ 1`), which was dropped in this patch.

[Severity: High]
Like the calculation above, could `t` be 0 here leading to a division by zero
panic?

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