| Message ID | 1453200631-11818-1-git-send-email-malcolm.crossley@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-19 at 10:50 +0000, Malcolm Crossley wrote: > The average is calculated over the period of time from the last > xenpm report of the average CPU frequency. > > Reporting the average CPU frequency helps confirm the level of turbo > boost being achieved per CPU. > > Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> > --- > tools/misc/xenpm.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c > index 08f2242..40cdd55 100644 > --- a/tools/misc/xenpm.c > +++ b/tools/misc/xenpm.c > @@ -45,6 +45,8 @@ void show_help(void) > "xenpm command list:\n\n" > " get-cpuidle-states [cpuid] list cpu idle info of > CPU <cpuid> or all\n" > " get-cpufreq-states [cpuid] list cpu freq info of > CPU <cpuid> or all\n" > + " get-cpufreq-average [cpuid] average cpu frequency > since last invocation\n" > + " for CPU <cpuid> or > all\n" > " get-cpufreq-para [cpuid] list cpu freq > parameter of CPU <cpuid> or all\n" > " set-scaling-maxfreq [cpuid] <HZ> set max cpu frequency > <HZ> on CPU <cpuid>\n" > " or all CPUs\n" > @@ -296,6 +298,9 @@ static int get_pxstat_by_cpuid(xc_interface > *xc_handle, int cpuid, struct xc_px_ > return ret; > } > > + > +static int *avgfreq; Do we need the global array of them all when each element is used exactly once entirely within show_cpufreq_by_cpuid in this mode?
On 19/01/16 11:06, Ian Campbell wrote: > On Tue, 2016-01-19 at 10:50 +0000, Malcolm Crossley wrote: >> The average is calculated over the period of time from the last >> xenpm report of the average CPU frequency. >> >> Reporting the average CPU frequency helps confirm the level of turbo >> boost being achieved per CPU. >> >> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> >> --- >> tools/misc/xenpm.c | 42 +++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c >> index 08f2242..40cdd55 100644 >> --- a/tools/misc/xenpm.c >> +++ b/tools/misc/xenpm.c >> @@ -45,6 +45,8 @@ void show_help(void) >> "xenpm command list:\n\n" >> " get-cpuidle-states [cpuid] list cpu idle info of >> CPU <cpuid> or all\n" >> " get-cpufreq-states [cpuid] list cpu freq info of >> CPU <cpuid> or all\n" >> + " get-cpufreq-average [cpuid] average cpu frequency >> since last invocation\n" >> + " for CPU <cpuid> or >> all\n" >> " get-cpufreq-para [cpuid] list cpu freq >> parameter of CPU <cpuid> or all\n" >> " set-scaling-maxfreq [cpuid] <HZ> set max cpu frequency >> <HZ> on CPU <cpuid>\n" >> " or all CPUs\n" >> @@ -296,6 +298,9 @@ static int get_pxstat_by_cpuid(xc_interface >> *xc_handle, int cpuid, struct xc_px_ >> return ret; >> } >> >> + >> +static int *avgfreq; > > Do we need the global array of them all when each element is used exactly > once entirely within show_cpufreq_by_cpuid in this mode? > The global array was already defined for when xenpm is used as a cpu frequency control daemon, I just moved it so that the new functions could use the variable. Do you want me to spin the patch using a local int variable instead? Malcolm
On 19/01/16 10:50, Malcolm Crossley wrote: > @@ -343,10 +348,44 @@ void pxstat_func(int argc, char *argv[]) > show_pxstat_by_cpuid(xc_handle, cpuid); > } > > +static int show_cpufreq_by_cpuid(xc_interface *xc_handle, int cpuid) > +{ > + int ret = 0; > + > + ret = get_avgfreq_by_cpuid(xc_handle, cpuid, &avgfreq[cpuid]); > + if ( ret ) > + return ret; > + > + printf("cpu id : %d\n", cpuid); > + printf("average cpu frequency: %d\n", avgfreq[cpuid]); > + printf("\n"); > + return 0; > +} > + > +void cpufreq_func(int argc, char *argv[]) > +{ > + int cpuid = -1; > + > + if ( argc > 0 ) > + parse_cpuid(argv[0], &cpuid); > + > + avgfreq = calloc(max_cpu_nr, sizeof(*avgfreq)); You need a NULL check here. ~Andrew
On Tue, 2016-01-19 at 11:10 +0000, Malcolm Crossley wrote: > On 19/01/16 11:06, Ian Campbell wrote: > > On Tue, 2016-01-19 at 10:50 +0000, Malcolm Crossley wrote: > > > The average is calculated over the period of time from the last > > > xenpm report of the average CPU frequency. > > > > > > Reporting the average CPU frequency helps confirm the level of turbo > > > boost being achieved per CPU. > > > > > > Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> > > > --- > > > tools/misc/xenpm.c | 42 +++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c > > > index 08f2242..40cdd55 100644 > > > --- a/tools/misc/xenpm.c > > > +++ b/tools/misc/xenpm.c > > > @@ -45,6 +45,8 @@ void show_help(void) > > > "xenpm command list:\n\n" > > > " get-cpuidle-states [cpuid] list cpu idle info > > > of > > > CPU <cpuid> or all\n" > > > " get-cpufreq-states [cpuid] list cpu freq info > > > of > > > CPU <cpuid> or all\n" > > > + " get-cpufreq-average [cpuid] average cpu > > > frequency > > > since last invocation\n" > > > + " for CPU <cpuid> or > > > all\n" > > > " get-cpufreq-para [cpuid] list cpu freq > > > parameter of CPU <cpuid> or all\n" > > > " set-scaling-maxfreq [cpuid] <HZ> set max cpu > > > frequency > > > <HZ> on CPU <cpuid>\n" > > > " or all CPUs\n" > > > @@ -296,6 +298,9 @@ static int get_pxstat_by_cpuid(xc_interface > > > *xc_handle, int cpuid, struct xc_px_ > > > return ret; > > > } > > > > > > + > > > +static int *avgfreq; > > > > Do we need the global array of them all when each element is used > > exactly > > once entirely within show_cpufreq_by_cpuid in this mode? > > > > The global array was already defined for when xenpm is used as a cpu frequency control daemon, I > just moved it so that the new functions could use the variable. > > Do you want me to spin the patch using a local int variable instead? I was going to say yes please, but I see v2 already arrived with that change ;-) Ian.
On 19/01/16 11:35, Ian Campbell wrote: > On Tue, 2016-01-19 at 11:10 +0000, Malcolm Crossley wrote: >> On 19/01/16 11:06, Ian Campbell wrote: >>> On Tue, 2016-01-19 at 10:50 +0000, Malcolm Crossley wrote: >>>> The average is calculated over the period of time from the last >>>> xenpm report of the average CPU frequency. >>>> >>>> Reporting the average CPU frequency helps confirm the level of turbo >>>> boost being achieved per CPU. >>>> >>>> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> >>>> --- >>>> tools/misc/xenpm.c | 42 +++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 41 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c >>>> index 08f2242..40cdd55 100644 >>>> --- a/tools/misc/xenpm.c >>>> +++ b/tools/misc/xenpm.c >>>> @@ -45,6 +45,8 @@ void show_help(void) >>>> "xenpm command list:\n\n" >>>> " get-cpuidle-states [cpuid] list cpu idle info >>>> of >>>> CPU <cpuid> or all\n" >>>> " get-cpufreq-states [cpuid] list cpu freq info >>>> of >>>> CPU <cpuid> or all\n" >>>> + " get-cpufreq-average [cpuid] average cpu >>>> frequency >>>> since last invocation\n" >>>> + " for CPU <cpuid> or >>>> all\n" >>>> " get-cpufreq-para [cpuid] list cpu freq >>>> parameter of CPU <cpuid> or all\n" >>>> " set-scaling-maxfreq [cpuid] <HZ> set max cpu >>>> frequency >>>> <HZ> on CPU <cpuid>\n" >>>> " or all CPUs\n" >>>> @@ -296,6 +298,9 @@ static int get_pxstat_by_cpuid(xc_interface >>>> *xc_handle, int cpuid, struct xc_px_ >>>> return ret; >>>> } >>>> >>>> + >>>> +static int *avgfreq; >>> >>> Do we need the global array of them all when each element is used >>> exactly >>> once entirely within show_cpufreq_by_cpuid in this mode? >>> >> >> The global array was already defined for when xenpm is used as a cpu frequency control daemon, I >> just moved it so that the new functions could use the variable. >> >> Do you want me to spin the patch using a local int variable instead? > > I was going to say yes please, but I see v2 already arrived with that > change ;-) It fixed Andrew's review comment at the same time and it makes for a smaller simpler patch. Thanks for the review. Malcolm > > Ian. >
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index 08f2242..40cdd55 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -45,6 +45,8 @@ void show_help(void) "xenpm command list:\n\n" " get-cpuidle-states [cpuid] list cpu idle info of CPU <cpuid> or all\n" " get-cpufreq-states [cpuid] list cpu freq info of CPU <cpuid> or all\n" + " get-cpufreq-average [cpuid] average cpu frequency since last invocation\n" + " for CPU <cpuid> or all\n" " get-cpufreq-para [cpuid] list cpu freq parameter of CPU <cpuid> or all\n" " set-scaling-maxfreq [cpuid] <HZ> set max cpu frequency <HZ> on CPU <cpuid>\n" " or all CPUs\n" @@ -296,6 +298,9 @@ static int get_pxstat_by_cpuid(xc_interface *xc_handle, int cpuid, struct xc_px_ return ret; } + +static int *avgfreq; + /* show cpu actual average freq information on CPU cpuid */ static int get_avgfreq_by_cpuid(xc_interface *xc_handle, int cpuid, int *avgfreq) { @@ -343,10 +348,44 @@ void pxstat_func(int argc, char *argv[]) show_pxstat_by_cpuid(xc_handle, cpuid); } +static int show_cpufreq_by_cpuid(xc_interface *xc_handle, int cpuid) +{ + int ret = 0; + + ret = get_avgfreq_by_cpuid(xc_handle, cpuid, &avgfreq[cpuid]); + if ( ret ) + return ret; + + printf("cpu id : %d\n", cpuid); + printf("average cpu frequency: %d\n", avgfreq[cpuid]); + printf("\n"); + return 0; +} + +void cpufreq_func(int argc, char *argv[]) +{ + int cpuid = -1; + + if ( argc > 0 ) + parse_cpuid(argv[0], &cpuid); + + avgfreq = calloc(max_cpu_nr, sizeof(*avgfreq)); + if ( cpuid < 0 ) + { + /* show average frequency on all cpus */ + int i; + for ( i = 0; i < max_cpu_nr; i++ ) + if ( show_cpufreq_by_cpuid(xc_handle, i) == -ENODEV ) + break; + } + else + show_cpufreq_by_cpuid(xc_handle, cpuid); + free(avgfreq); +} + static uint64_t usec_start, usec_end; static struct xc_cx_stat *cxstat, *cxstat_start, *cxstat_end; static struct xc_px_stat *pxstat, *pxstat_start, *pxstat_end; -static int *avgfreq; static uint64_t *sum, *sum_cx, *sum_px; static void signal_int_handler(int signo) @@ -1129,6 +1168,7 @@ struct { { "help", help_func }, { "get-cpuidle-states", cxstat_func }, { "get-cpufreq-states", pxstat_func }, + { "get-cpufreq-average", cpufreq_func }, { "start", start_gather_func }, { "get-cpufreq-para", cpufreq_para_func }, { "set-scaling-maxfreq", scaling_max_freq_func },
The average is calculated over the period of time from the last xenpm report of the average CPU frequency. Reporting the average CPU frequency helps confirm the level of turbo boost being achieved per CPU. Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> --- tools/misc/xenpm.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)