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: <0d0f0528-738b-402c-a05c-53e21000dc67@kylinos.cn>
Date: Wed, 28 Jan 2026 09:44:40 +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/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);

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