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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <56eb86c0-9b60-4f7f-8824-16f14a019e3c@kylinos.cn>
Date: Wed, 28 Jan 2026 17:20:55 +0800
From: Feng Jiang <jiangfeng@...inos.cn>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: pjw@...nel.org, palmer@...belt.com, aou@...s.berkeley.edu, alex@...ti.fr,
 akpm@...ux-foundation.org, kees@...nel.org, andy@...nel.org,
 ebiggers@...nel.org, martin.petersen@...cle.com, sohil.mehta@...el.com,
 charlie@...osinc.com, conor.dooley@...rochip.com, samuel.holland@...ive.com,
 linus.walleij@...aro.org, nathan@...nel.org,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH v5 4/8] lib/string_kunit: add performance benchmark for
 strlen()

On 2026/1/28 16:59, Andy Shevchenko wrote:
> 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.

Thanks for the feedback. I'll incorporate this change, along with the other
improvements we discussed, and send out v6 shortly.

>> 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,
Feng Jiang