| Message ID | 20180910150400.11907-1-prarit@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | [v2] cpupower: Add better frequency-info failure messaging | expand |
ping -- old patch? Any comments? /me may have missed something P. On 9/10/18 11:04 AM, Prarit Bhargava wrote: > Tested with: > > Performance Per Watt (OS) (ie, kernel) > > current policy: frequency should be within 800 MHz and 3.00 GHz. > The governor "performance" may decide which speed to use > within this range. > current CPU frequency: 2.03 GHz (asserted by call to kernel) > > Performance Per Watt (DAPC) (ie, niether) > > current policy: frequency should be within 800 MHz and 3.00 GHz. > The governor "performance" may decide which speed to use > within this range. > current CPU frequency: Unable to call hardware > current CPU frequency: Unable to call kernel > > Performance (ie, HW) > > current policy: frequency should be within 800 MHz and 3.00 GHz. > The governor "performance" may decide which speed to use > within this range. > current CPU frequency: 2.33 GHz (asserted by call to kernel) > > ----8<---- > > If a Dell system has "OS DBPM(Performance Per Watt OS)" set in BIOS, > cpupower frequency-info outputs > > current CPU frequency: Unable to call hardware > current CPU frequency: 2.50 GHz (asserted by call to kernel) > > to indicate that cpupower cannot read the frequency from hardware and > is using the kernel's cpu frequency estimate. This output is confusing > to end users who wonder why the hardware is not responding. > > Update the cpupower frequency-info output message to only output the > error if both the hardware and kernel calls fail. > > v2: Update logic and output separate error lines on failure. > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > Cc: Thomas Renninger <trenn@suse.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Stafford Horne <shorne@gmail.com> > --- > tools/power/cpupower/utils/cpufreq-info.c | 36 +++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/tools/power/cpupower/utils/cpufreq-info.c b/tools/power/cpupower/utils/cpufreq-info.c > index df43cd45d810..8c39f735745b 100644 > --- a/tools/power/cpupower/utils/cpufreq-info.c > +++ b/tools/power/cpupower/utils/cpufreq-info.c > @@ -246,17 +246,19 @@ static int get_boost_mode(unsigned int cpu) > > /* --freq / -f */ > > -static int get_freq_kernel(unsigned int cpu, unsigned int human) > +static int get_freq_kernel(unsigned int cpu, unsigned int human, int fail_msg) > { > unsigned long freq = cpufreq_get_freq_kernel(cpu); > - printf(_(" current CPU frequency: ")); > + > if (!freq) { > - printf(_(" Unable to call to kernel\n")); > + if (fail_msg) > + printf(_(" current CPU frequency: Unable to call to kernel\n")); > return -EINVAL; > } > - if (human) { > + printf(_(" current CPU frequency: ")); > + if (human) > print_speed(freq); > - } else > + else > printf("%lu", freq); > printf(_(" (asserted by call to kernel)\n")); > return 0; > @@ -265,17 +267,19 @@ static int get_freq_kernel(unsigned int cpu, unsigned int human) > > /* --hwfreq / -w */ > > -static int get_freq_hardware(unsigned int cpu, unsigned int human) > +static int get_freq_hardware(unsigned int cpu, unsigned int human, int fail_msg) > { > unsigned long freq = cpufreq_get_freq_hardware(cpu); > - printf(_(" current CPU frequency: ")); > + > if (!freq) { > - printf("Unable to call hardware\n"); > + if (fail_msg) > + printf(_(" current CPU frequency: Unable to call hardware\n")); > return -EINVAL; > } > - if (human) { > + printf(_(" current CPU frequency: ")); > + if (human) > print_speed(freq); > - } else > + else > printf("%lu", freq); > printf(_(" (asserted by call to hardware)\n")); > return 0; > @@ -475,8 +479,12 @@ static void debug_output_one(unsigned int cpu) > > get_available_governors(cpu); > get_policy(cpu); > - if (get_freq_hardware(cpu, 1) < 0) > - get_freq_kernel(cpu, 1); > + if (get_freq_hardware(cpu, 1, 0)) { > + if (get_freq_kernel(cpu, 1, 0)) { > + printf(_(" current CPU frequency: Unable to call hardware\n")); > + printf(_(" current CPU frequency: Unable to call kernel\n")); > + } > + } > get_boost_mode(cpu); > } > > @@ -627,10 +635,10 @@ int cmd_freq_info(int argc, char **argv) > ret = get_hardware_limits(cpu, human); > break; > case 'w': > - ret = get_freq_hardware(cpu, human); > + ret = get_freq_hardware(cpu, human, 1); > break; > case 'f': > - ret = get_freq_kernel(cpu, human); > + ret = get_freq_kernel(cpu, human, 1); > break; > case 's': > ret = get_freq_stats(cpu, human); >
On Monday, December 3, 2018 3:56:29 PM CET Prarit Bhargava wrote: > ping -- old patch? Any comments? > > /me may have missed something Looks fine. Only enhancement would have been to keep the original, translated printf(_""); Instead of: > > - printf(_(" current CPU frequency: ")); > > - printf(_(" Unable to call to kernel\n")); > > + printf(_(" current CPU frequency: Unable to call to kernel\n")); keep the individual messages by 2 separate printfs. But the message changed already over time: tools/power/cpupower/po/cs.po:msgid " current CPU frequency is " So do not mind..., at some point there should be a release and some translation effort would make sense then. Reviewed-by: Thomas Renninger <trenn@suse.de> (for the original patch as sent). Thomas
Hi Prarit, On 12/4/18 4:28 AM, Thomas Renninger wrote: > On Monday, December 3, 2018 3:56:29 PM CET Prarit Bhargava wrote: >> ping -- old patch? Any comments? >> >> /me may have missed something > > Looks fine. > Only enhancement would have been to keep the original, translated > printf(_""); > Instead of: >>> - printf(_(" current CPU frequency: ")); >>> - printf(_(" Unable to call to kernel\n")); >>> + printf(_(" current CPU frequency: Unable to call to kernel\n")); > keep the individual messages by 2 separate printfs. > But the message changed already over time: > tools/power/cpupower/po/cs.po:msgid " current CPU frequency is " Would like to spin the patch and make this change Thomas suggested? I would like to get this in for 4.21-rc1 > > So do not mind..., at some point there should be a release and some > translation effort would make sense then. > > Reviewed-by: Thomas Renninger <trenn@suse.de> > (for the original patch as sent). thanks, -- Shuah
diff --git a/tools/power/cpupower/utils/cpufreq-info.c b/tools/power/cpupower/utils/cpufreq-info.c index df43cd45d810..8c39f735745b 100644 --- a/tools/power/cpupower/utils/cpufreq-info.c +++ b/tools/power/cpupower/utils/cpufreq-info.c @@ -246,17 +246,19 @@ static int get_boost_mode(unsigned int cpu) /* --freq / -f */ -static int get_freq_kernel(unsigned int cpu, unsigned int human) +static int get_freq_kernel(unsigned int cpu, unsigned int human, int fail_msg) { unsigned long freq = cpufreq_get_freq_kernel(cpu); - printf(_(" current CPU frequency: ")); + if (!freq) { - printf(_(" Unable to call to kernel\n")); + if (fail_msg) + printf(_(" current CPU frequency: Unable to call to kernel\n")); return -EINVAL; } - if (human) { + printf(_(" current CPU frequency: ")); + if (human) print_speed(freq); - } else + else printf("%lu", freq); printf(_(" (asserted by call to kernel)\n")); return 0; @@ -265,17 +267,19 @@ static int get_freq_kernel(unsigned int cpu, unsigned int human) /* --hwfreq / -w */ -static int get_freq_hardware(unsigned int cpu, unsigned int human) +static int get_freq_hardware(unsigned int cpu, unsigned int human, int fail_msg) { unsigned long freq = cpufreq_get_freq_hardware(cpu); - printf(_(" current CPU frequency: ")); + if (!freq) { - printf("Unable to call hardware\n"); + if (fail_msg) + printf(_(" current CPU frequency: Unable to call hardware\n")); return -EINVAL; } - if (human) { + printf(_(" current CPU frequency: ")); + if (human) print_speed(freq); - } else + else printf("%lu", freq); printf(_(" (asserted by call to hardware)\n")); return 0; @@ -475,8 +479,12 @@ static void debug_output_one(unsigned int cpu) get_available_governors(cpu); get_policy(cpu); - if (get_freq_hardware(cpu, 1) < 0) - get_freq_kernel(cpu, 1); + if (get_freq_hardware(cpu, 1, 0)) { + if (get_freq_kernel(cpu, 1, 0)) { + printf(_(" current CPU frequency: Unable to call hardware\n")); + printf(_(" current CPU frequency: Unable to call kernel\n")); + } + } get_boost_mode(cpu); } @@ -627,10 +635,10 @@ int cmd_freq_info(int argc, char **argv) ret = get_hardware_limits(cpu, human); break; case 'w': - ret = get_freq_hardware(cpu, human); + ret = get_freq_hardware(cpu, human, 1); break; case 'f': - ret = get_freq_kernel(cpu, human); + ret = get_freq_kernel(cpu, human, 1); break; case 's': ret = get_freq_stats(cpu, human);
Tested with: Performance Per Watt (OS) (ie, kernel) current policy: frequency should be within 800 MHz and 3.00 GHz. The governor "performance" may decide which speed to use within this range. current CPU frequency: 2.03 GHz (asserted by call to kernel) Performance Per Watt (DAPC) (ie, niether) current policy: frequency should be within 800 MHz and 3.00 GHz. The governor "performance" may decide which speed to use within this range. current CPU frequency: Unable to call hardware current CPU frequency: Unable to call kernel Performance (ie, HW) current policy: frequency should be within 800 MHz and 3.00 GHz. The governor "performance" may decide which speed to use within this range. current CPU frequency: 2.33 GHz (asserted by call to kernel) ----8<---- If a Dell system has "OS DBPM(Performance Per Watt OS)" set in BIOS, cpupower frequency-info outputs current CPU frequency: Unable to call hardware current CPU frequency: 2.50 GHz (asserted by call to kernel) to indicate that cpupower cannot read the frequency from hardware and is using the kernel's cpu frequency estimate. This output is confusing to end users who wonder why the hardware is not responding. Update the cpupower frequency-info output message to only output the error if both the hardware and kernel calls fail. v2: Update logic and output separate error lines on failure. Signed-off-by: Prarit Bhargava <prarit@redhat.com> Cc: Thomas Renninger <trenn@suse.com> Cc: Shuah Khan <shuah@kernel.org> Cc: Stafford Horne <shorne@gmail.com> --- tools/power/cpupower/utils/cpufreq-info.c | 36 +++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-)