Re: [PATCH] xenpm: Add option to report average CPU frequency

[Malcolm Crossley <malcolm.crossley@citrix.com>]

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.
>