|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
> The intel_pstate driver receives percentage values to set the
> performance limits. This patch adds interfaces to support the
> input of percentage values to control the intel_pstate driver.
> Also, the "get-cpufreq-para" is modified to show percentage
> based feedback info.
>
This patch does more than "adding the interface". See below.
> v4 changes:
> None.
>
> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx>
> ---
> tools/libxc/include/xenctrl.h | 14 ++++-
> tools/libxc/xc_pm.c | 17 ++++---
> tools/misc/xenpm.c | 116
> +++++++++++++++++++++++++++++++++---------
> 3 files changed, 115 insertions(+), 32 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 100b89c..a79494a 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
> uint32_t scaling_cur_freq;
>
> char scaling_governor[CPUFREQ_NAME_LEN];
> - uint32_t scaling_max_freq;
> - uint32_t scaling_min_freq;
> +
> + union {
> + uint32_t freq;
> + uint32_t pct;
> + } scaling_max;
> +
> + union {
> + uint32_t freq;
> + uint32_t pct;
> + } scaling_min;
> +
Don't you need struct? I don't see how union would work for you, you
clearly need bot freq and pct at the same time.
> + uint32_t scaling_turbo_pct;
>
> /* for specific governor */
> union {
> diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
> index 823bab6..300de33 100644
> --- a/tools/libxc/xc_pm.c
> +++ b/tools/libxc/xc_pm.c
> @@ -261,13 +261,16 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> }
> else
> {
> - user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
> - user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
> - user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
> - user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
> - user_para->scaling_max_freq = sys_para->scaling_max.freq;
> - user_para->scaling_min_freq = sys_para->scaling_min.freq;
> - user_para->turbo_enabled = sys_para->turbo_enabled;
> + user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
> + user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
> + user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
> + user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
> + user_para->scaling_max.freq = sys_para->scaling_max.freq;
> + user_para->scaling_min.freq = sys_para->scaling_min.freq;
> + user_para->scaling_max.pct = sys_para->scaling_max.pct;
> + user_para->scaling_min.pct = sys_para->scaling_min.pct;
> + user_para->scaling_turbo_pct =
> sys_para->scaling_turbo_pct;
> + user_para->turbo_enabled = sys_para->turbo_enabled;
The new indentation looks wrong. You don't need so many spaces.
>
> memcpy(user_para->scaling_driver,
> sys_para->scaling_driver, CPUFREQ_NAME_LEN);
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index 2f9bd8e..ea6a32f 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -33,6 +33,11 @@
> #define MAX_CORE_RESIDENCIES 8
>
> #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
> +#define min_t(type,x,y) \
> + ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
> +#define max_t(type,x,y) \
> + ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
> +#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
>
> static xc_interface *xc_handle;
> static unsigned int max_cpu_nr;
> @@ -47,6 +52,9 @@ void show_help(void)
> " 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-para [cpuid] list cpu freq parameter of
> CPU <cpuid> or all\n"
> + " set-scaling-max-pct [cpuid] <num> set max performance limit
> in percentage\n"
> + " or as scaling speed in
> percentage in userspace governor\n"
> + " set-scaling-min-pct [cpuid] <num> set min performance limit
> in percentage\n"
> " set-scaling-maxfreq [cpuid] <HZ> set max cpu frequency <HZ>
> on CPU <cpuid>\n"
> " or all CPUs\n"
> " set-scaling-minfreq [cpuid] <HZ> set min cpu frequency <HZ>
> on CPU <cpuid>\n"
> @@ -60,10 +68,10 @@ void show_help(void)
> " set-up-threshold [cpuid] <num> set up threshold on CPU
> <cpuid> or all\n"
> " it is used in ondemand
> governor.\n"
> " get-cpu-topology get thread/core/socket
> topology info\n"
> - " set-sched-smt enable|disable enable/disable
> scheduler smt power saving\n"
> + " set-sched-smt enable|disable
> enable/disable scheduler smt power saving\n"
Please add a line in commit message saying that you also fixed some
indentation issues in help string.
Preferably all indentation changes (including the ones below) should be
separated from functional changes.
> " set-vcpu-migration-delay <num> set scheduler vcpu
> migration delay in us\n"
> " get-vcpu-migration-delay get scheduler vcpu
> migration delay\n"
> - " set-max-cstate <num> set the C-State limitation
> (<num> >= 0)\n"
> + " set-max-cstate <num> set the C-State limitation
> (<num> >= 0)\n"
> " start [seconds] start collect Cx/Px
> statistics,\n"
> " output after CTRL-C or
> SIGINT or several seconds.\n"
> " enable-turbo-mode [cpuid] enable Turbo Mode for
> processors that support it.\n"
> @@ -678,38 +686,50 @@ static void print_cpufreq_para(int cpuid, struct
> xc_get_cpufreq_para *p_cpufreq)
>
> printf("current_governor : %s\n", p_cpufreq->scaling_governor);
> if ( !strncmp(p_cpufreq->scaling_governor,
> - "userspace", CPUFREQ_NAME_LEN) )
> + "userspace", CPUFREQ_NAME_LEN) &&
> + strncmp(p_cpufreq->scaling_driver,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> {
> - printf(" userspace specific :\n");
> - printf(" scaling_setspeed : %u\n",
> + printf("userspace specific :\n");
> + printf("scaling_setspeed : %u\n",
And this is?
If the change in indentation is necessary and agreed upon, that's fine
by me. Otherwise this is not necessary.
If this change stays, you also need to say that in commit message,
something like "fix (improve?) the output by moving indentation".
> p_cpufreq->u.userspace.scaling_setspeed);
> }
> else if ( !strncmp(p_cpufreq->scaling_governor,
> - "ondemand", CPUFREQ_NAME_LEN) )
> + "ondemand", CPUFREQ_NAME_LEN) &&
> + strncmp(p_cpufreq->scaling_driver,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> {
> - printf(" ondemand specific :\n");
> - printf(" sampling_rate : max [%u] min [%u] cur [%u]\n",
> + printf("ondemand specific :\n");
> + printf("sampling_rate : max [%u] min [%u] cur [%u]\n",
> p_cpufreq->u.ondemand.sampling_rate_max,
> p_cpufreq->u.ondemand.sampling_rate_min,
> p_cpufreq->u.ondemand.sampling_rate);
> - printf(" up_threshold : %u\n",
> + printf("up_threshold : %u\n",
> p_cpufreq->u.ondemand.up_threshold);
> }
>
> - printf("scaling_avail_freq :");
> - for ( i = 0; i < p_cpufreq->freq_num; i++ )
> - if ( p_cpufreq->scaling_available_frequencies[i] ==
> - p_cpufreq->scaling_cur_freq )
> - printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
> - else
> - printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
> - printf("\n");
> -
> - printf("scaling frequency : max [%u] min [%u] cur [%u]\n",
> - p_cpufreq->scaling_max_freq,
> - p_cpufreq->scaling_min_freq,
> - p_cpufreq->scaling_cur_freq);
> -
> + if (!strncmp(p_cpufreq->scaling_driver,
Coding style, space after '(' please.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |