| Message ID | 704cfb6696840b3838576ea583b8ab8ed2265aaf.1499858779.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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
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
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.
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
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.
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
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"
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(+)