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
