| Message ID | 20150511154022.GA3390@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On Monday, May 11, 2015 12:40:22 PM Herton R. Krzesinski wrote: > On Mon, May 11, 2015 at 11:35:35AM -0300, Herton R. Krzesinski wrote: > > There is clearly wrong output when mperf monitor runs in MAX_FREQ_SYSFS mode: > > average frequency shows in kHz unit (despite the intended output to be in MHz), > > and percentages for C state information are all wrong (including high/negative > > values shown). > > > > The problem is that the max_frequency read on initialization isn't used where it > > should have been used on mperf_get_count_percent (to estimate the number of > > ticks in the given time period), and the value we read from sysfs is in kHz, so > > we must divide it to get the MHz value to use in current calculations. > > > > While at it, also I fixed another small issues in the debug output of > > max_frequency value in mperf_get_count_freq. > > > > Signed-off-by: Herton R. Krzesinski <herton@redhat.com> > > --- > > tools/power/cpupower/utils/idle_monitor/mperf_monitor.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > Actually please consider v2 patch below, just a minor change in the debug > output, which isn't a percentage... Thomas, any comments? > From: "Herton R. Krzesinski" <herton@redhat.com> > Date: Mon, 11 May 2015 11:18:14 -0300 > Subject: [PATCH v2] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode > > There is clearly wrong output when mperf monitor runs in MAX_FREQ_SYSFS mode: > average frequency shows in kHz unit (despite the intended output to be in MHz), > and percentages for C state information are all wrong (including high/negative > values shown). > > The problem is that the max_frequency read on initialization isn't used where it > should have been used on mperf_get_count_percent (to estimate the number of > ticks in the given time period), and the value we read from sysfs is in kHz, so > we must divide it to get the MHz value to use in current calculations. > > While at it, also I fixed another small issues in the debug output of > max_frequency value in mperf_get_count_freq. > > Signed-off-by: Herton R. Krzesinski <herton@redhat.com> > --- > tools/power/cpupower/utils/idle_monitor/mperf_monitor.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > v2: remove percent from debug output fix > > diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c > index 90a8c4f..c83f160 100644 > --- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c > +++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c > @@ -135,7 +135,7 @@ static int mperf_get_count_percent(unsigned int id, double *percent, > dprint("%s: TSC Ref - mperf_diff: %llu, tsc_diff: %llu\n", > mperf_cstates[id].name, mperf_diff, tsc_diff); > } else if (max_freq_mode == MAX_FREQ_SYSFS) { > - timediff = timespec_diff_us(time_start, time_end); > + timediff = max_frequency * timespec_diff_us(time_start, time_end); > *percent = 100.0 * mperf_diff / timediff; > dprint("%s: MAXFREQ - mperf_diff: %llu, time_diff: %llu\n", > mperf_cstates[id].name, mperf_diff, timediff); > @@ -176,7 +176,7 @@ static int mperf_get_count_freq(unsigned int id, unsigned long long *count, > dprint("%s: Average freq based on %s maximum frequency:\n", > mperf_cstates[id].name, > (max_freq_mode == MAX_FREQ_TSC_REF) ? "TSC calculated" : "sysfs read"); > - dprint("%max_frequency: %lu", max_frequency); > + dprint("max_frequency: %lu\n", max_frequency); > dprint("aperf_diff: %llu\n", aperf_diff); > dprint("mperf_diff: %llu\n", mperf_diff); > dprint("avg freq: %llu\n", *count); > @@ -279,6 +279,7 @@ use_sysfs: > return -1; > } > max_freq_mode = MAX_FREQ_SYSFS; > + max_frequency /= 1000; /* Default automatically to MHz value */ > return 0; > } > >
Hi, and sorry for the late answer, this had not highest prio... On Wednesday, May 20, 2015 02:26:47 AM Rafael J. Wysocki wrote: > On Monday, May 11, 2015 12:40:22 PM Herton R. Krzesinski wrote: > > On Mon, May 11, 2015 at 11:35:35AM -0300, Herton R. Krzesinski wrote: > > > There is clearly wrong output when mperf monitor runs in MAX_FREQ_SYSFS > > > mode: average frequency shows in kHz unit (despite the intended output > > > to be in MHz), and percentages for C state information are all wrong > > > (including high/negative values shown). > > > > > > The problem is that the max_frequency read on initialization isn't used > > > where it should have been used on mperf_get_count_percent (to estimate > > > the number of ticks in the given time period), and the value we read > > > from sysfs is in kHz, so we must divide it to get the MHz value to use > > > in current calculations. > > > > > > While at it, also I fixed another small issues in the debug output of > > > max_frequency value in mperf_get_count_freq. > > > > > > Signed-off-by: Herton R. Krzesinski <herton@redhat.com> > > > --- > > > > > > tools/power/cpupower/utils/idle_monitor/mperf_monitor.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > Actually please consider v2 patch below, just a minor change in the debug > > output, which isn't a percentage... > > Thomas, any comments? I tried to find fitting HW without much success. The MAX_FREQ_SYSFS is used only on rare HW and it may never got a proper test. Patch looks sane. Even I cannot add a Tested-by: you may add: Acked-by: Thomas Renninger <trenn@suse.de> and add it. Thanks, Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c index 90a8c4f..c83f160 100644 --- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c +++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c @@ -135,7 +135,7 @@ static int mperf_get_count_percent(unsigned int id, double *percent, dprint("%s: TSC Ref - mperf_diff: %llu, tsc_diff: %llu\n", mperf_cstates[id].name, mperf_diff, tsc_diff); } else if (max_freq_mode == MAX_FREQ_SYSFS) { - timediff = timespec_diff_us(time_start, time_end); + timediff = max_frequency * timespec_diff_us(time_start, time_end); *percent = 100.0 * mperf_diff / timediff; dprint("%s: MAXFREQ - mperf_diff: %llu, time_diff: %llu\n", mperf_cstates[id].name, mperf_diff, timediff); @@ -176,7 +176,7 @@ static int mperf_get_count_freq(unsigned int id, unsigned long long *count, dprint("%s: Average freq based on %s maximum frequency:\n", mperf_cstates[id].name, (max_freq_mode == MAX_FREQ_TSC_REF) ? "TSC calculated" : "sysfs read"); - dprint("%max_frequency: %lu", max_frequency); + dprint("max_frequency: %lu\n", max_frequency); dprint("aperf_diff: %llu\n", aperf_diff); dprint("mperf_diff: %llu\n", mperf_diff); dprint("avg freq: %llu\n", *count); @@ -279,6 +279,7 @@ use_sysfs: return -1; } max_freq_mode = MAX_FREQ_SYSFS; + max_frequency /= 1000; /* Default automatically to MHz value */ return 0; }