Re: [PATCH 4/8] xor/kunit: add a benchmark

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Potential divide-by-zero kernel panic in `xor_benchmark()` when calculating the benchmark speed if the execution time is 0.
- [Medium] The benchmark holds `preempt_disable()` for the entire duration of 1000 XOR iterations, risking soft lockups on slower hardware or emulators.
--

--- Patch [4]: [PATCH 4/8] xor/kunit: add a benchmark ---
commit 4f82691a9522c9ab25bdc94cf0c1ad74dc8c3b7e
Author: Christoph Hellwig <[email protected]>

xor/kunit: add a benchmark

This patch adds a benchmark to test the XOR functions for more
representative block sizes and numbers of disks. It measures the execution
time of 1000 iterations of xor_gen() for varying buffer counts and lengths,
reporting the throughput in GB/s.

> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25b..648c6da9464c4 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c

[ ... ]

> @@ -125,8 +125,70 @@ static void xor_test(struct kunit *test)
>  	}
>  }
>  
> +static void xor_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, XOR_KUNIT_MAX_BYTES);
> +
> +			preempt_disable();
> +			t = ktime_get_ns();
> +			for (l = 0; l < num_iters; l++)
> +				xor_gen(test_dest, test_buffers, nr, len);
> +			t = ktime_get_ns() - t;
> +			preempt_enable();

[Severity: Medium]
Does this workload risk triggering the kernel's soft lockup detector?

The test executes a hardcoded 1000 iterations of xor_gen() on up to 32 buffers
of 16KB within a single preempt_disable() block. This amounts to processing
over 500MB of data atomically.

In slower environments like QEMU TCG without KVM, this large computational
workload could take tens of seconds, which might exceed the typical 20-second
soft lockup threshold.

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

[Severity: High]
Can this result in a divide-by-zero kernel panic?

If the benchmark runs on a system with coarse timekeeping resolution (e.g.,
using the jiffies clocksource), and the CPU completes the 1000 iterations
within a single timer tick, the elapsed time `t` could evaluate to 0.

Since `t` is used directly as the divisor in div64_u64() without being
checked, this might trigger a hardware exception and panic the kernel.

> +		}
> +
> +		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/[email protected]?part=4