On 2026/1/27 16:57, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 09:25:54AM +0800, Feng Jiang wrote:
>> Introduce a benchmarking framework to the string_kunit test suite to
>> measure the execution efficiency of string functions.
>>
>> The implementation is inspired by crc_benchmark(), measuring throughput
>> (MB/s) and latency (ns/call) across a range of string lengths. It
>> includes a warm-up phase, disables preemption during measurement, and
>> uses a fixed seed for reproducible results.
>>
>> This framework allows for comparing different implementations (e.g.,
>> generic C vs. architecture-optimized assembly) within the KUnit
>> environment.
>
> Acked-by: Andy Shevchenko <[email protected]>
> A few nit-picks below.
>
Thanks for the Acked-by and the detailed review.
> ...
>
>> +static void *alloc_max_bench_buffer(struct kunit *test,
>> + const size_t *lens, size_t count, size_t *buf_len)
>> +{
>> + size_t max_len = 0;
>> + void *buf;
>> +
>> + for (size_t i = 0; i < count; i++)
>> + max_len = max(lens[i], max_len);
>
> You also need minmax.h.
Will add.
> ...
>
>> +#define STRING_BENCH(iters, func, ...)
>> \
>> +({ \
>> + /* Volatile function pointer prevents dead code elimination */ \
>> + typeof(func) (* volatile __func) = (func); \
>> + size_t __bn_iters = (iters); \
>> + size_t __bn_warm_iters; \
>> + u64 __bn_t; \
>
>
> Perhaps a short comment here
>
> /* Use 10% of the given iterations (maximum 50) to warm up */
Will add this comment.
>> + __bn_warm_iters = max(__bn_iters / 10, 50U); \
>> + \
>> + for (size_t __bn_i = 0; __bn_i < __bn_warm_iters; __bn_i++) \
>> + (void)__func(__VA_ARGS__); \
>> + \
>> + preempt_disable(); \
>> + __bn_t = ktime_get_ns(); \
>> + for (size_t __bn_i = 0; __bn_i < __bn_iters; __bn_i++) \
>> + (void)__func(__VA_ARGS__); \
>> + __bn_t = ktime_get_ns() - __bn_t; \
>> + preempt_enable(); \
>> + __bn_t; \
>> +})
>
> ...
>
>> +#define STRING_BENCH_BUF(test, buf_name, buf_size, func, ...)
>> \
>> +do {
>> \
>> + size_t buf_size, _bn_i, _bn_iters, _bn_size = 0; \
>> + u64 _bn_t, _bn_mbps = 0, _bn_lat = 0; \
>> + char *buf_name, *_bn_buf; \
>> + \
>> + _bn_buf = alloc_max_bench_buffer(test, bench_lens, \
>> + ARRAY_SIZE(bench_lens), &_bn_size); \
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, _bn_buf); \
>> + \
>> + fill_random_string(_bn_buf, _bn_size); \
>> + \
>> + for (_bn_i = 0; _bn_i < ARRAY_SIZE(bench_lens); _bn_i++) { \
>> + buf_size = bench_lens[_bn_i]; \
>> + buf_name = _bn_buf + _bn_size - buf_size - 1; \
>> + _bn_iters = STRING_BENCH_WORKLOAD / max(buf_size, 1U); \
>> + \
>> + _bn_t = STRING_BENCH(_bn_iters, func, ##__VA_ARGS__); \
>
>> + \
>
> Remove unneeded blank line.
Will fix.
>> + if (_bn_t > 0) { \
>> + _bn_mbps = (u64)(buf_size) * _bn_iters \
>
> Why buf_size in the parentheses here and not anywhere else (above)?
It was a bit inconsistent. I'll remove the unneeded parentheses for buf_size.
> I assume it's just an external temporary variable? But why do we need to have
> it in the parameters to the macro?
This is necessary because buf_size often needs to be passed as an argument
to the function under test. For instance, when benchmarking strnlen, the
caller must pass the current length as an argument:
STRING_BENCH_BUF(test, buf, len, strnlen, buf, len);
>> + * (NSEC_PER_SEC / MEGA); \
> > Leave '*' on the previous line.
Will fix.
>> + _bn_mbps = div64_u64(_bn_mbps, _bn_t); \
>> + _bn_lat = div64_u64(_bn_t, _bn_iters); \
>> + } \
>> + kunit_info(test, "len=%zu: %llu MB/s (%llu ns/call)\n", \
>> + buf_size, _bn_mbps, _bn_lat); \
>> + } \
>> +} while (0)
>
Thank you again for your patience and for helping me improve the quality of
this patch.
--
With Best Regards,
Feng Jiang
