* [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode @ 2015-05-11 14:35 Herton R. Krzesinski 2015-05-11 15:40 ` Herton R. Krzesinski 0 siblings, 1 reply; 4+ messages in thread From: Herton R. Krzesinski @ 2015-05-11 14:35 UTC (permalink / raw) To: Thomas Renninger Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Herton R. Krzesinski 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(-) diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c index 90a8c4f..369d49ca 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; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode 2015-05-11 14:35 [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode Herton R. Krzesinski @ 2015-05-11 15:40 ` Herton R. Krzesinski 2015-05-20 0:26 ` Rafael J. Wysocki 0 siblings, 1 reply; 4+ messages in thread From: Herton R. Krzesinski @ 2015-05-11 15:40 UTC (permalink / raw) To: Thomas Renninger; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel 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... 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; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode 2015-05-11 15:40 ` Herton R. Krzesinski @ 2015-05-20 0:26 ` Rafael J. Wysocki 2015-05-27 13:24 ` Thomas Renninger 0 siblings, 1 reply; 4+ messages in thread From: Rafael J. Wysocki @ 2015-05-20 0:26 UTC (permalink / raw) To: Herton R. Krzesinski, Thomas Renninger Cc: Rafael J. Wysocki, linux-pm, linux-kernel 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; > } > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode 2015-05-20 0:26 ` Rafael J. Wysocki @ 2015-05-27 13:24 ` Thomas Renninger 0 siblings, 0 replies; 4+ messages in thread From: Thomas Renninger @ 2015-05-27 13:24 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Herton R. Krzesinski, Rafael J. Wysocki, linux-pm, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-05-27 13:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-11 14:35 [PATCH] cpupower: mperf monitor: fix output in MAX_FREQ_SYSFS mode Herton R. Krzesinski 2015-05-11 15:40 ` Herton R. Krzesinski 2015-05-20 0:26 ` Rafael J. Wysocki 2015-05-27 13:24 ` Thomas Renninger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox