Re: [Xen-devel] [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c

2015-11-20 Thread Jan Beulich 
>>> On 28.10.15 at 04:21,  wrote:
>  tools/libxc/include/xenctrl.h  |  20 ++--
>  tools/libxc/xc_pm.c|  16 ++--

I won't (again) comment on the changes of these files (I'll leave it
to the tools maintainers), but I can't help thinking that there's more
shuffling there than necessary.

> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct 
> xc_get_cpufreq_para *p_cpufreq)
>  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_max_perf,
> +   p_cpufreq->scaling_min_perf,
> p_cpufreq->scaling_cur_freq);

So where did your percentages go?

> @@ -240,40 +253,85 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>  if ( ret )
>  return ret;
>  
> -if ( !(scaling_available_governors =
> -   xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> -return -ENOMEM;
> -if ( (ret = read_scaling_available_governors(scaling_available_governors,
> -gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
> +if ( internal_gov )
>  {
> +ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> +internal_gov->avail_gov, gov_num * CPUFREQ_NAME_LEN);
> +if ( ret )
> +return ret;
> +}
> +else
> +{
> +if ( !(scaling_available_governors =
> +   xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> +return -ENOMEM;
> +if ( (ret = 
> read_scaling_available_governors(scaling_available_governors,
> +gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
> +{
> +xfree(scaling_available_governors);
> +return ret;
> +}
> +ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> +scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
>  xfree(scaling_available_governors);
> -return ret;
> +if ( ret )
> +return ret;
>  }

Even if the benefit may look tiny, code that can be shared should be
shared. In the case here, the conditional return should be pulled out
of the if and else branches. (It in fact seems possible to also pull out
the copy_to_guest(), which would result in quite a bit less churn on
the code.)

> -op->u.get_para.scaling_max_freq = policy->max;
> -op->u.get_para.scaling_min_freq = policy->min;
> +if ( internal_gov )
> +{
> +op->u.get_para.scaling_max_perf = limits->max_perf_pct;
> +op->u.get_para.scaling_min_perf = limits->min_perf_pct;
> +op->u.get_para.scaling_turbo_pct = limits->turbo_pct;
> +if ( !strncmp(cpufreq_driver->name,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> +op->u.get_para.perf_alias = PERCENTAGE;
> +else
> +op->u.get_para.perf_alias = FREQUENCY;

Shouldn't the internal governor tell you, rather than keying this
on its name?

> @@ -299,16 +357,36 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>  static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
>  {
>  struct cpufreq_policy new_policy, *old_policy;
> +struct internal_governor *internal_gov;
>  
>  old_policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>  if ( !old_policy )
>  return -EINVAL;
> +internal_gov = old_policy->internal_gov;
>  
>  memcpy(_policy, old_policy, sizeof(struct cpufreq_policy));
>  
> -new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
> -if (new_policy.governor == NULL)
> -return -EINVAL;
> +if ( internal_gov && internal_gov->cur_gov )

Either the right side of the && has to go away (preferred), or it has
to become != NON_INTERNAL_GOV.

> +{
> +if ( !strnicmp(op->u.set_gov.scaling_governor,
> +   "performance", CPUFREQ_NAME_LEN) )
> +internal_gov->cur_gov = INTERNAL_GOV_PERFORMANCE;
> +else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +   "powersave", CPUFREQ_NAME_LEN) )
> +internal_gov->cur_gov = INTERNAL_GOV_POWERSAVE;
> +else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +   "userspace", CPUFREQ_NAME_LEN) )
> +internal_gov->cur_gov = INTERNAL_GOV_USERSPACE;
> +else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +   "ondemand", CPUFREQ_NAME_LEN) )
> +internal_gov->cur_gov = INTERNAL_GOV_ONDEMAND;

else ...

> @@ -317,10 +395,12 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>  {
>  int ret = 0;
>  struct cpufreq_policy *policy;
> +struct internal_governor *internal_gov;
>  
>  policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> +internal_gov = policy->internal_gov;
>  
> -

Re: [Xen-devel] [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c

2015-10-28 Thread Wei Liu 
On Wed, Oct 28, 2015 at 11:21:17AM +0800, Wei Wang wrote:
> Add support in the pmstat.c so that the xenpm tool can request to
> access the intel_pstate driver.
> 
> Signed-off-by: Wei Wang 
> ---
>  changes in v6:
>  1) add the NON_INTERNAL_GOV macro to replace literal 0;
>  2) code consolidation (e.g. merging some code into if/else, as required in 
> v5);
>  3) somewhere, change to use clamp, instead of clamp_t;
>  4) xen_perf_alias, instead of perf_alias.
> 
>  tools/libxc/include/xenctrl.h  |  20 ++--
>  tools/libxc/xc_pm.c|  16 ++--
>  tools/misc/xenpm.c |   4 +-

I think all changes to toolstack code are merely trying to mirror
hypervisor side changes and there are no functional changes.

With that understanding and subject to an ack from hypervisor side
maintainer:

Acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c

2015-10-27 Thread Wei Wang 
Add support in the pmstat.c so that the xenpm tool can request to
access the intel_pstate driver.

Signed-off-by: Wei Wang 
---
 changes in v6:
 1) add the NON_INTERNAL_GOV macro to replace literal 0;
 2) code consolidation (e.g. merging some code into if/else, as required in v5);
 3) somewhere, change to use clamp, instead of clamp_t;
 4) xen_perf_alias, instead of perf_alias.

 tools/libxc/include/xenctrl.h  |  20 ++--
 tools/libxc/xc_pm.c|  16 ++--
 tools/misc/xenpm.c |   4 +-
 xen/drivers/acpi/pmstat.c  | 183 +++--
 xen/include/acpi/cpufreq/cpufreq.h |   2 +
 xen/include/public/sysctl.h|  29 --
 6 files changed, 198 insertions(+), 56 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bfa00b..590eb72 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2270,6 +2270,17 @@ struct xc_get_cpufreq_para {
 uint32_t cpu_num;
 uint32_t freq_num;
 uint32_t gov_num;
+int32_t turbo_enabled;
+
+uint32_t cpuinfo_cur_freq;
+uint32_t cpuinfo_max_freq;
+uint32_t cpuinfo_min_freq;
+uint32_t scaling_cur_freq;
+
+uint32_t scaling_turbo_pct;
+uint32_t scaling_max_perf;
+uint32_t scaling_min_perf;
+enum xen_perf_alias perf_alias;
 
 /* for all governors */
 /* OUT variable */
@@ -2278,22 +2289,13 @@ struct xc_get_cpufreq_para {
 char *scaling_available_governors;
 char scaling_driver[CPUFREQ_NAME_LEN];
 
-uint32_t cpuinfo_cur_freq;
-uint32_t cpuinfo_max_freq;
-uint32_t cpuinfo_min_freq;
-uint32_t scaling_cur_freq;
-
 char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
 
 /* for specific governor */
 union {
 xc_userspace_t userspace;
 xc_ondemand_t ondemand;
 } u;
-
-int32_t turbo_enabled;
 };
 
 int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index 5b38cf1..6a16e8a 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -260,13 +260,15 @@ 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_perf  = sys_para->scaling_max_perf;
+user_para->scaling_min_perf  = sys_para->scaling_min_perf;
+user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct;
+user_para->perf_alias= sys_para->perf_alias;
+user_para->turbo_enabled = sys_para->turbo_enabled;
 
 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 08f2242..5944fdb 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct 
xc_get_cpufreq_para *p_cpufreq)
 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_max_perf,
+   p_cpufreq->scaling_min_perf,
p_cpufreq->scaling_cur_freq);
 
 printf("turbo mode   : %s\n",
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index 892260d..7825f91 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -191,7 +191,9 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
 uint32_t ret = 0;
 const struct processor_pminfo *pmpt;
 struct cpufreq_policy *policy;
-uint32_t gov_num = 0;
+struct perf_limits *limits;
+struct internal_governor *internal_gov;
+uint32_t cur_gov, gov_num = 0;
 uint32_t *affected_cpus;
 uint32_t *scaling_available_frequencies;
 char *scaling_available_governors;
@@ -200,13 +202,24 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
 
 pmpt = processor_pminfo[op->cpuid];
 policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
+limits = >limits;
+internal_gov = policy->internal_gov;
 
 if ( !pmpt || !pmpt->perf.states ||
- !policy || !policy->governor )
+ !policy || (!policy->governor && !internal_gov) )