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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 04:21, <wei.w.wang@xxxxxxxxx> 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(&new_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;
>  
> -    if ( !policy || !policy->governor )
> +    if ( !policy || (!policy->governor && !internal_gov) )
>          return -EINVAL;
>  
>      switch(op->u.set_para.ctrl_type)
> @@ -329,6 +409,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>      {
>          struct cpufreq_policy new_policy;
>  
> +        if ( !policy->governor || internal_gov )
> +            return -EINVAL;

The "!policy->governor" part looks to be dead code, considering the
check done in the previous hunk.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;
>   * same as sysfs file name of native linux
>   */
>  #define CPUFREQ_NAME_LEN 16
> +
> +enum xen_perf_alias {
> +    FREQUENCY  = 0,
> +    PERCENTAGE = 1
> +};

The need proper prefixes to avoid name space collisions, e.g.
XEN_CPUFREQ_MODE_ (and the enum name being "alias" is
also odd, "mode" or some such would seem better there too).

>  struct xen_get_cpufreq_para {
>      /* IN/OUT variable */
>      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;

You shouldn't use fields of non-fixed type in the public interface.

> @@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {
>      XEN_GUEST_HANDLE_64(uint32) scaling_available_frequencies;
>      XEN_GUEST_HANDLE_64(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 {
>          struct  xen_userspace userspace;
>          struct  xen_ondemand ondemand;
>      } u;
> -
> -    int32_t turbo_enabled;
>  };

Just to repeat myself: Too much shuffling for no reason. I can see
why you want turbo_enabled go up (to reduce holes), but I don't
see the point of any of the other moving around of members.

Jan

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