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
