Re: [PATCH v5 4/8] lib/string_kunit: add performance benchmark for strlen()

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Feng Jiang <jiangfeng@kylinos.cn>
Cc: pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu,
	alex@ghiti.fr, akpm@linux-foundation.org, kees@kernel.org,
	andy@kernel.org, ebiggers@kernel.org, martin.petersen@oracle.com,
	sohil.mehta@intel.com, charlie@rivosinc.com,
	conor.dooley@microchip.com, samuel.holland@sifive.com,
	linus.walleij@linaro.org, nathan@kernel.org,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v5 4/8] lib/string_kunit: add performance benchmark for strlen()
Date: Wed, 28 Jan 2026 10:59:43 +0200	[thread overview]
Message-ID: <aXnP_6wsyXcVGasN@smile.fi.intel.com> (raw)
In-Reply-To: <0d0f0528-738b-402c-a05c-53e21000dc67@kylinos.cn>

On Wed, Jan 28, 2026 at 09:44:40AM +0800, Feng Jiang wrote:
> On 2026/1/27 17:50, Andy Shevchenko wrote:
> > On Tue, Jan 27, 2026 at 05:33:10PM +0800, Feng Jiang wrote:
> >> On 2026/1/27 16:57, Andy Shevchenko wrote:
> >>> On Tue, Jan 27, 2026 at 09:25:54AM +0800, Feng Jiang wrote:

...

> >>>> +#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);
> > 
> > Okay, and why is it needed in this macro? It get overridden immediately in
> > the loop. Assuming that the array size of bench lengths is not zero, this
> > has no visible use. Can you elaborate?
> 
> Thank you for the explanation. I see the source of the confusion now.
> 
> In v5, buf_name and buf_size were not intended to pass external variables into
> the macro. Instead, they were naming placeholders for local variables declared
> inside the macro's scope. This allows the caller to define the names used in
> the variadic arguments.
> 
> To resolve the logical inconsistency you pointed out, I'd like to propose two
> options for v6. Which one would you prefer?
> 
> Option 1: Internal Declaration (The "Self-Contained" approach)
> 
> We declare and initialize the variables directly inside the loop. This keeps
> the macro self-contained and the caller doesn't need to pre-declare anything.
> 
> for (_bn_i = 0; _bn_i < ARRAY_SIZE(bench_lens); _bn_i++) {
>     size_t buf_size = bench_lens[_bn_i];
>     char *buf_name = _bn_buf + _bn_size - buf_size - 1;
>     ...
> }
> 
> Usage:
>   STRING_BENCH_BUF(test, my_buf, my_len, strnlen, my_buf, my_len);

This option is better as long as the user doesn't need to know the (stale) data
out of these parameters. And I think this is the case, so #1 is the winner.

> Option 2: External Declaration (The list.h approach)
> 
> The macro expects the caller to provide pre-declared variables, similar to
> list_for_each_entry(). This removes all re-declarations inside the macro.
> 
> 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;
>     ...
> }
> 
> Usage:
>   size_t my_len;
>   char *my_buf;
>   STRING_BENCH_BUF(test, my_buf, my_len, strnlen, my_buf, my_len);
> 
> Please let me know which style fits the kernel's preference better, and
> I will implement it in v6 along with your other suggestions.
> 
> Thanks for the catch!
> 
> >>>> +					* (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)

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv