Re: [PATCH v2 2/2] lib/string_kunit: extend benchmarks and unit test to memcmp()

[Andy Shevchenko]

On Thu, May 14, 2026 at 7:19 PM Kees Cook <k...@kernel.org> wrote:
> On Thu, May 14, 2026 at 02:13:58PM +0200, Milan Tripkovic wrote:
> > From: Milan Tripkovic <milan.tripko...@rt-rk.com>
> >
> > Extend the string benchmarking suite to include memcmp().
> > Extend the string unit test to include memcmp().

> > Reported-by: kernel test robot <l...@intel.com>
> > Closes: 
> > https://lore.kernel.org/oe-kbuild-all/202605140827.qg1dzpcb-...@intel.com/

What does this mean? The absence of the test cases won't ever be
reported by LKP.

...

> > +#if IS_ENABLED(CONFIG_STRING_KUNIT_BENCH)
> > +static void string_bench_memcmp(struct kunit *test)

> I think I'd prefer, instead of a ifdef stub, to do:
>
>         if (!IS_ENABLED(CONFIG_STRING_KUNIT_BENCH))
>                 kunit_skip(test, "CONFIG_STRING_KUNIT_BENCH not enabled")
>
> here.

What about having two functions?

static void do_test_string_bench_memcmp() // choose better name
{
  ...
}

static void string_bench_memcmp(struct kunit *test)
{
        if (IS_ENABLED(CONFIG_STRING_KUNIT_BENCH))
                do_test_string_bench_memcmp(...);
        else
                kunit_skip(test, "CONFIG_STRING_KUNIT_BENCH not enabled");
}

...

> > +     for (int o = 0; o < ARRAY_SIZE(offsets); o++) {
> > +             int off = offsets[o];
> > +
> > +             for (int i = 0; i < ARRAY_SIZE(lengths); i++) {

unsigned int i

> > +                     size_t len = lengths[i];
> > +                     char *p1 = buf1;
> > +                     char *p2 = buf2 + off;

> > +

Redundant blank line

> > +                     u32 iterations = (len < 512) ? 100000 : 10000;
> > +
> > +                     for (u32 j = 0; j < iterations; j++) {
> > +                             (void)memcmp(p1, p2, len);
> > +                             barrier();
> > +                     }

> > +                     u64 elapsed = STRING_BENCH(iterations, memcmp, p1, 
> > p2, len);
> > +                     u64 ns_per_call = div_u64(elapsed, iterations);
> > +                     u64 mbps = len ? div_u64((u64)len * iterations * 1000,

Instead of casting just make iterations to be u64.

                     mbps = len ? div_u64(iterations * 1000 * len,

> > +                                                                      
> > elapsed) : 0;

We don't usually allow the C99 definition initialisers inside the
code, even for tests.

(With that the last will be a single line.)

> > +                     if (off == 0) {
> > +                             kunit_info(test, "bench_memcmp_aligned: 
> > len=%-4zu: %llu MB/s (%llu ns/call)\n",
> > +                                        len, mbps, ns_per_call);
> > +                     } else {
> > +                             kunit_info(test, 
> > "bench_memcmp_unaligned(off=%d): len=%-4zu: %llu MB/s (%llu ns/call)\n",
> > +                                        off, len, mbps, ns_per_call);
> > +                     }
> > +             }
> > +     }

-- 
With Best Regards,
Andy Shevchenko