Re: [PATCH 8/8] raid6/kunit: add a benchmark

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Potential division by zero in benchmark speed calculation if elapsed time `t` evaluates to zero.
- [Medium] Unbounded atomic section (preemption disabled) can cause soft lockups or RCU stalls on slower architectures.
- [Low] Duplicate `RAID6_PQ` Kconfig symbol declaration.
- [Low] Inaccurate commit message referencing XOR functions instead of RAID6 PQ functions.
--

--- Patch [8]: [PATCH 8/8] raid6/kunit: add a benchmark ---
commit 63d135afc471564206ce7921d64339a16ff1484f
Author: Christoph Hellwig <hch@lst.de>

raid6/kunit: add a benchmark
    
This commit adds a benchmark to the RAID6 KUnit test suite to evaluate P/Q
generation speeds across different disk counts and block sizes. It splits
the max alloc size from the kunit max size and reports the performance in
GB/s.

> Add a benchmark to test the XOR functions for more representative block
> sizes and numbers of disks.  This splits the maximum alloc size used for

[Severity: Low]
This isn't a bug, but the commit message states it adds a benchmark for the
"XOR functions", while the patch actually adds a benchmark for the RAID6 P/Q
generation algorithm.

[ ... ]
> diff --git a/lib/raid/Kconfig b/lib/raid/Kconfig
> index c4eeb7c716c89..c52d5c2052946 100644
> --- a/lib/raid/Kconfig
> +++ b/lib/raid/Kconfig
> @@ -60,6 +60,15 @@ config RAID6_PQ_KUNIT_TEST
>  	  This is intended to help people writing architecture-specific
>  	  optimized versions.  If unsure, say N.
>  
> +config RAID6_PQ_KUNIT_BENCHMARK
> +	bool "Benchmark for RAID6 PQ"
> +	depends on RAID6_PQ_KUNIT_TEST
> +	help
> +	  Include benchmarks in the KUnit test suite for raid P/Q generation.
> +
> +config RAID6_PQ
> +	tristate
> +

[Severity: Low]
Is this duplicate declaration of config RAID6_PQ intentional? It appears to
be defined earlier in this same file.

>  config RAID6_PQ_BENCHMARK
>  	bool "Automatically choose fastest RAID6 PQ functions"
>  	depends on RAID6_PQ
> diff --git a/lib/raid/raid6/tests/raid6_kunit.c b/lib/raid/raid6/tests/raid6_kunit.c
> index 9f3e671a12241..f927e3456d175 100644
> --- a/lib/raid/raid6/tests/raid6_kunit.c
> +++ b/lib/raid/raid6/tests/raid6_kunit.c
[ ... ]
> @@ -229,6 +230,68 @@ static void raid6_test(struct kunit *test)
>  		raid6_test_one(test);
>  }
>  
> +static void raid6_benchmark(struct kunit *test)
> +{
[ ... ]
> +		for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> +			unsigned int len = len_to_test[j];
> +			const unsigned long num_iters = 1000;
> +
> +			KUNIT_ASSERT_GT(test, len, 0);
> +			KUNIT_ASSERT_LE(test, len, RAID6_KUNIT_ALLOC_BYTES);
> +
> +			preempt_disable();
> +			t = ktime_get_ns();
> +			for (l = 0; l < num_iters; l++)
> +				raid6_gen_syndrome(nr_to_test[i],
> +						len_to_test[j], test_buffers);
> +			t = ktime_get_ns() - t;
> +			preempt_enable();

[Severity: Medium]
Could this unbounded atomic section cause soft lockups or RCU stalls on
slower architectures or emulators? 

The loop executes 1,000 iterations of raid6_gen_syndrome() with preemption
disabled. On slow hardware or CI emulators, processing this much data might
take multiple seconds and prevent the scheduler from running.

> +
> +			speed[j] = div64_u64((u64)len * num_iters * nr, t);

[Severity: Medium]
Is there a risk of a division by zero panic here?

If the KUnit test runs on a system with a coarse-resolution clock (such as
with CONFIG_HIGH_RES_TIMERS disabled) and a sufficiently fast CPU, the
1,000 iterations might complete in less time than a single clock tick.

In this scenario, ktime_get_ns() could return the same value before and
after the loop, causing the elapsed time t to evaluate to exactly 0 before
being passed as the divisor.

> +		}
> +
> +		static_assert(ARRAY_SIZE(len_to_test) == 2);
> +		kunit_info(test, "%3u disks:\t%5llu  GB/s\t%5llu  GB/s\n",
> +				nr, speed[0], speed[1]);
> +	}
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=8