[PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Leonard Crestez]

This checks that the cpufreq driver actually sets the requested
frequency.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---

I've been looking at using kselftests for imx. This patch exposes an
issue with the imx6 cpufreq driver on imx6sx where frequencies are set
incorrectly because of clk mishandling. This is already caught by some
internal test scripts which also run against upstream but it's nice to
make this visible through kselftest.

I'm not sure it's correct to check that frequency matches exactly,
perhaps something like a 5% tolerance should be included for complex
drivers where the target freq is only a "hint"? I checked intel_pstate
but it doesn't even seem to expose an userspace governor for manual
frequency selection anyway.

Unfortunately cpufreq selftests don't seem to have a clear idea of
"pass" or "fail" results. This patch will just print some TAP-like
"ok" and "not ok" lines but failures are not actually propagated upwards
in a well-defined way.

Have you considered what it would take to TAP-ify the output of cpufreq
tests? Output is very complex so perhaps it might make sense to adopt some
sort of subtest syntax for kselftest, something like this:

http://tap4j.sourceforge.net/subtests.html

Sadly the TAP spec itself does not include support for subtests.

 tools/testing/selftests/cpufreq/cpufreq.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/cpufreq/cpufreq.sh b/tools/testing/selftests/cpufreq/cpufreq.sh
index 1ed3832..323b5bb 100755
--- a/tools/testing/selftests/cpufreq/cpufreq.sh
+++ b/tools/testing/selftests/cpufreq/cpufreq.sh
@@ -151,6 +151,14 @@ test_all_frequencies()
 	# Set all frequencies one-by-one
 	for freq in $freqs; do
 		set_cpu_frequency $1 $freq
+
+		local cur_freq
+		cur_freq=`cat $CPUFREQROOT/$1/cpuinfo_cur_freq`
+		if [ $freq -ne $cur_freq ]; then
+			printf "not ok - frequency set $freq but got $cur_freq instead!\n"
+		else
+			printf "ok - frequency check $freq\n"
+		fi
 	done
 
 	printf "\n"
-- 
2.7.4

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Rafael J. Wysocki]

On Wed, Jul 12, 2017 at 1:29 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> This checks that the cpufreq driver actually sets the requested
> frequency.

This won't work on modern x86  with APERF/MPERF (see recent commits in
that area).

Thanks,
Rafael

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Leonard Crestez]

On Wed, 2017-07-12 at 13:36 +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 12, 2017 at 1:29 PM, Leonard Crestez
> <leonard.crestez@nxp.com> wrote:
> > 
> > This checks that the cpufreq driver actually sets the requested
> > frequency.
> This won't work on modern x86  with APERF/MPERF (see recent commits in
> that area).

This test should be skipped if the cpu always adjusts it's own
frequency dynamically. It should check if scaling_set_speed won't
reflect in cpuinfo_cur_freq, but I'm not sure how to do that reliably.

I checked with the intel_pstate driver and it's already skipped in this
test because no userspace governor is available. This could be a way to
check if the CPU supports targetting a precise constant frequency.

Or are you saying that cpuinfo_cur_freq in general shouldn't be
expected to just return the current frequency but also adjust for time
spent in various idle states? It seems to me that if you want to track
the current load it might be better to report this through different
more explicit mechanism.

In particular imx already supports cpuidle states where the arm core
clock is turned off and later resumed but this has no effect on
cpuinfo_cur_freq.

--
Regards,
Leonard

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Rafael J. Wysocki]

On Wed, Jul 12, 2017 at 6:51 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> On Wed, 2017-07-12 at 13:36 +0200, Rafael J. Wysocki wrote:
>> On Wed, Jul 12, 2017 at 1:29 PM, Leonard Crestez
>> <leonard.crestez@nxp.com> wrote:
>> >
>> > This checks that the cpufreq driver actually sets the requested
>> > frequency.
>> This won't work on modern x86  with APERF/MPERF (see recent commits in
>> that area).
>
> This test should be skipped if the cpu always adjusts it's own
> frequency dynamically. It should check if scaling_set_speed won't
> reflect in cpuinfo_cur_freq, but I'm not sure how to do that reliably.

Well, this is my concern, actually.

> I checked with the intel_pstate driver and it's already skipped in this
> test because no userspace governor is available. This could be a way to
> check if the CPU supports targetting a precise constant frequency.
>
> Or are you saying that cpuinfo_cur_freq in general shouldn't be
> expected to just return the current frequency but also adjust for time
> spent in various idle states?

On x86 cpuinfo_cur_freq shouldn't be expected to return the frequency
requested by the driver.

After the recent changes, if the APERF and MPERF feedback registers
are available, arch code will use them to compute the current
frequency as seen by the CPU which is not guaranteed to be equal to
whatever has been requested by the cpufreq driver for various reasons.

> It seems to me that if you want to track
> the current load it might be better to report this through different
> more explicit mechanism.
>
> In particular imx already supports cpuidle states where the arm core
> clock is turned off and later resumed but this has no effect on
> cpuinfo_cur_freq.

I guess you could just blacklist x86 overall in this test.

Thanks,
Rafael

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Viresh Kumar]

On 12-07-17, 14:29, Leonard Crestez wrote:
> This checks that the cpufreq driver actually sets the requested
> frequency.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> 
> I've been looking at using kselftests for imx. This patch exposes an
> issue with the imx6 cpufreq driver on imx6sx where frequencies are set
> incorrectly because of clk mishandling. This is already caught by some
> internal test scripts which also run against upstream but it's nice to
> make this visible through kselftest.

Sure, thanks for that.

> I'm not sure it's correct to check that frequency matches exactly,
> perhaps something like a 5% tolerance should be included for complex
> drivers where the target freq is only a "hint"?

We can do better, see below..

> I checked intel_pstate
> but it doesn't even seem to expose an userspace governor for manual
> frequency selection anyway.

Sure, and so that wouldn't be affected by this.

> Unfortunately cpufreq selftests don't seem to have a clear idea of
> "pass" or "fail" results.

Yeah, I had this test setup for a while and just pushed it through.
Over that many tests aren't really tests but just looking out for
crashes, etc. Never got a chance to improve it :(

> This patch will just print some TAP-like
> "ok" and "not ok" lines but failures are not actually propagated upwards
> in a well-defined way.

That would be fine for now.

> Have you considered what it would take to TAP-ify the output of cpufreq
> tests? Output is very complex so perhaps it might make sense to adopt some
> sort of subtest syntax for kselftest, something like this:

Not yet :(

> diff --git a/tools/testing/selftests/cpufreq/cpufreq.sh b/tools/testing/selftests/cpufreq/cpufreq.sh
> index 1ed3832..323b5bb 100755
> --- a/tools/testing/selftests/cpufreq/cpufreq.sh
> +++ b/tools/testing/selftests/cpufreq/cpufreq.sh
> @@ -151,6 +151,14 @@ test_all_frequencies()
>  	# Set all frequencies one-by-one
>  	for freq in $freqs; do
>  		set_cpu_frequency $1 $freq
> +
> +		local cur_freq
> +		cur_freq=`cat $CPUFREQROOT/$1/cpuinfo_cur_freq`

Yes, we want to verify if freq change happened or not, but may be only
reading scaling_cur_freq would be enough for now?

And that wouldn't be a problem for X86 (which Rafael mentioned) as
well IIUC.

-- 
viresh

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Leonard Crestez]

On Thu, 2017-07-13 at 14:25 +0530, Viresh Kumar wrote:
> On 12-07-17, 14:29, Leonard Crestez wrote:
> > 
> > This checks that the cpufreq driver actually sets the requested
> > frequency.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > 
> > ---
> > 
> > I've been looking at using kselftests for imx. This patch exposes an
> > issue with the imx6 cpufreq driver on imx6sx where frequencies are set
> > incorrectly because of clk mishandling. This is already caught by some
> > internal test scripts which also run against upstream but it's nice to
> > make this visible through kselftest.
> Sure, thanks for that.
> 
> > I'm not sure it's correct to check that frequency matches exactly,
> > perhaps something like a 5% tolerance should be included for complex
> > drivers where the target freq is only a "hint"?
> We can do better, see below..
> 
> > I checked intel_pstate
> > but it doesn't even seem to expose an userspace governor for manual
> > frequency selection anyway.
> Sure, and so that wouldn't be affected by this.
> 
> > diff --git a/tools/testing/selftests/cpufreq/cpufreq.sh
> > b/tools/testing/selftests/cpufreq/cpufreq.sh
> > index 1ed3832..323b5bb 100755
> > --- a/tools/testing/selftests/cpufreq/cpufreq.sh
> > +++ b/tools/testing/selftests/cpufreq/cpufreq.sh
> > @@ -151,6 +151,14 @@ test_all_frequencies()
> >  	# Set all frequencies one-by-one
> >  	for freq in $freqs; do
> >  		set_cpu_frequency $1 $freq
> > +
> > +		local cur_freq
> > +		cur_freq=`cat $CPUFREQROOT/$1/cpuinfo_cur_freq`
> Yes, we want to verify if freq change happened or not, but may be only
> reading scaling_cur_freq would be enough for now?
> 
> And that wouldn't be a problem for X86 (which Rafael mentioned) as
> well IIUC.
> 
The semantics of scaling_cur_freq and cpuinfo_cur_freq are not very
clear to me.

In my particular case I need to check cpuinfo_cur_freq because this is
what ends up returning the rate of the arm clk. Otherwise
scaling_cur_freq just returns policy->cur unless the driver has a
setpolicy function (I don't understand that condition).

Since commit f8475cef9008 ("x86: use common aperfmperf_khz_on_cpu() to
calculate KHz using APERF/MPERF") scaling_cur_freq will also try to
return the "computed frequency" on x86.

I ran selftest with this patch on top of upstream on an AMD Phenom
machine (scaling_driver="acpi_cpufreq") and it still passes. It returns
the value computed through aperf/mperf in scaling_cur_freq but manual
explicit switching between supported frequencies is reflected in
cpuinfo_cur_freq, as the test expects.

I'm not sure this means that cpuinfo_cur_freq is the correct choice, it
seems like it works mostly by accident rather than ABI guarantees. I
suspect that if people actually attempt to run this test on a wide
variety of systems it will need an endless stream of platform-specific
hacks to pass. Better to let this die.

--
Regards,
Leonard

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Viresh Kumar]

On 18-07-17, 22:34, Leonard Crestez wrote:
> The semantics of scaling_cur_freq and cpuinfo_cur_freq are not very
> clear to me.

cpuinfo_cur_freq reads the frequency right from hardware all the time
and so can be slow. It can only be read by root if I remember
correctly.

Whereas scaling_cur_freq tries to read the cached frequency. But it
has changed a bit with the below mentioned patch.

> In my particular case I need to check cpuinfo_cur_freq because this is
> what ends up returning the rate of the arm clk. Otherwise
> scaling_cur_freq just returns policy->cur

Yeah, we may actually need to use cpuinfo_cur_freq as that is what
ends up giving the real freq.

> unless the driver has a
> setpolicy function (I don't understand that condition).

That's because the core doesn't know the cached freq for setpolicy
drivers and so we need to call the ->get() callback. But for non
setpolicy drivers, core already has the cached value.

-- 
viresh

Re: [PATCH] selftests: cpufreq: Check cpuinfo_cur_freq set as expected

[Rafael J. Wysocki]

On Wednesday, July 19, 2017 12:24:06 PM Viresh Kumar wrote:
> On 18-07-17, 22:34, Leonard Crestez wrote:
> > The semantics of scaling_cur_freq and cpuinfo_cur_freq are not very
> > clear to me.
> 
> cpuinfo_cur_freq reads the frequency right from hardware all the time
> and so can be slow. It can only be read by root if I remember
> correctly.
> 
> Whereas scaling_cur_freq tries to read the cached frequency. But it
> has changed a bit with the below mentioned patch.
> 
> > In my particular case I need to check cpuinfo_cur_freq because this is
> > what ends up returning the rate of the arm clk. Otherwise
> > scaling_cur_freq just returns policy->cur
> 
> Yeah, we may actually need to use cpuinfo_cur_freq as that is what
> ends up giving the real freq.
> 
> > unless the driver has a
> > setpolicy function (I don't understand that condition).
> 
> That's because the core doesn't know the cached freq for setpolicy
> drivers and so we need to call the ->get() callback. But for non
> setpolicy drivers, core already has the cached value.

Please remember that cpuinfo_cur_freq may not be present.

Thanks,
Rafael