Re: [PATCH/RFC] lib: add CPU MHz benchmark test

[Willy Tarreau]

Hi Geert,

On Wed, Jan 31, 2024 at 05:46:48PM +0100, Geert Uytterhoeven wrote:
> When working on SoC bring-up, (a full) userspace may not be available,
> making it hard to benchmark the CPU performance of the system under
> development.  Still, one may want to have a rough idea of the (relative)
> performance of one or more CPU cores, especially when working on e.g.
> the clock driver that controls the CPU core clock(s).
> 
> Hence add the CPU MHz benchmark test[1], which estimates the clock
> frequency of the CPU core it is running on, and make it available as a
> Linux kernel test module.
> 
> When built-in, this benchmark can be run without any userspace present.

That's a great idea, I never thought about turning it into a module!

> Parallel runs (run on multiple CPU cores) are supported, just kick the
> "run" file multiple times.

Hmmm does it mean it will run on the CPU that writes this "run" ?
Because this could allow one to start tests using e.g.:

    taskset -c $CPU tee /sys/.../run <<< y

But we could also wonder if it wouldn't be easier to either send "y"
to /sys/.../cpu0/run or may just send the CPU number to "run" instead
of "y". In my experience with this tool, you most always want to easily
control the CPU number because SoCs these days are not symmetrical at
all.

> This has been tested on the folowing CPU cores:
>   - ARM: Cortex A7, A9, and A15,
>   - ARM64: Cortex A53, A55, A57, and A76,
>   - m68k: MC68040,
>   - MIPS: TX4927,
>   - RISC-V: AndesTech AX45, Kendryte K210, SiFive U54 and U74, VexRiscV.
>   - SuperH: SH7751R.
> The reported figures are usually within 1-2% of the actual CPU clock
> rate.

Nice!

> Known issues:
>   - The reported value is off on the following systems:
>       - RBTX4927: 120 MHz (should be 200 MHz, userspace mhz is OK)
> 	  user: count=76500 us50=19990 us250=96885 diff=76895 cpu_MHz=198.973
> 	  kernel:     43663      19943       93024                    119
> 	  msleep(1000) does sleep 1s, and ktime_get() advances accordingly
>       - RZ/Five: 1971 MHz (should be 1000 MHz, userspace mhz not tested)
> 	  kernel:    679625      20001       88962                   1971
> 	  msleep(1000) does sleep 1s, and ktime_get() advances accordingly
>       - VexRiscV: 12 MHz (should be 64 MHz, userspace mhz not tested)
>     I assume this is due to different optimization flags.
>     I haven't compared the generated code yet.

That's strange. I only ever managed to get off results at -O0, but at
-O1/-Og/-Os/-O2/-O3/-Ofast I've always got consistent results on all
the machines I've tested. Especially seeing results higher than the
real value is troubling. One possibility would be that one some archs
there's not enough registers and the compiler needs to use a variable
in the stack, but that could only explain the lower perf, not the higher
one. But indeed, it could be interesting to have a look at the code to
understand why it's doing that. If we figure that there's an inherent
limitation on some rare archs, in the worst case we could place a
warning there.

>   - On fast systems with a large clock granularity (e.g. ARAnyM running
>     Linux/m68k), the measured durations for the short and long loops may
>     be identical, causing division-by-zero exceptions.
>     The same happens with the userspace version, cfr.
>     https://github.com/wtarreau/mhz/issues/5.

I've responded there and we definitely need to address this, thanks!

Another point is that it would be nice if there was a way to present
the result in a form that a script can retrieve from the directory,
maybe the last measurement or something like this. I know that scripts
are commonly used to check for a machine's correct behavior, and I try
to encourage users to verify that it's working well, so anything we can
do that makes it easier to use would be welcome.

But overall, I like it! You've got my ack.

Hmmm I don't know if this is intended, the SPDX tag says MIT but the
MODULE_LICENSE at the top says MIT/GPL. I can't say I care that much but
I preferred to report it in case it's an accident ;-)

Thanks!
Willy