Re: [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.06.15 at 13:17, <wei.w.wang@intel.com> wrote:
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -192,22 +192,33 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      uint32_t ret = 0;
>      const struct processor_pminfo *pmpt;
>      struct cpufreq_policy *policy;
> +    struct perf_limits *limits;
> +    struct internal_governor *internal_gov;
>      uint32_t gov_num = 0;
>      uint32_t *affected_cpus;
>      uint32_t *scaling_available_frequencies;
>      char     *scaling_available_governors;
>      struct list_head *pos;
>      uint32_t cpu, i, j = 0;
> +    uint32_t cur_gov;

Please group this on the same line as e.g. gov_num, instead of adding
yet another line with a single uint32_t variable declaration.

> @@ -241,28 +252,47 @@ 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)

Coding style.

>      {
> +        scaling_available_governors = internal_gov->avail_gov;
> +        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> +                    scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);

Using scaling_available_governors as intermediate variable here
makes the code pretty clearly worse than just directly using
internal_gov->avail_gov. Or is there a problem doing so?

> @@ -270,11 +300,31 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      else
>          strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>  
> -    if ( policy->governor->name[0] )
> -        strlcpy(op->u.get_para.scaling_governor, 
> -            policy->governor->name, CPUFREQ_NAME_LEN);
> -    else
> -        strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
> +    switch (cur_gov)
> +    {
> +    case INTERNAL_GOV_PERFORMANCE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "performance", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_POWERSAVE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "powersave", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_USERSPACE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "userspace", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_ONDEMAND:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "ondemand", CPUFREQ_NAME_LEN);
> +        break;
> +    default:
> +        if ( policy->governor->name[0] )
> +            strlcpy(op->u.get_para.scaling_governor,
> +                policy->governor->name, CPUFREQ_NAME_LEN);

Indentation (for all second lines of strlcpy() invocations above).

> @@ -300,16 +350,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)
> +    {
> +        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
> +    {
> +        new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
> +        if (new_policy.governor == NULL)

Please not only fix the coding style of your own if(), but also that
of pre-existing code that you touch (like the one here).

> @@ -318,10 +388,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)
> @@ -348,6 +420,30 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>          break;
>      }
>  
> +    case SCALING_MAX_PCT:
> +    {
> +        struct cpufreq_policy new_policy;
> +        struct perf_limits *limits = &new_policy.limits;
> +
> +        new_policy = *policy;
> +        limits->max_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
> +			limits->min_policy_pct, limits->max_policy_pct);
> +        ret = __cpufreq_set_policy(policy, &new_policy);
> +        break;
> +    }
> +
> +    case SCALING_MIN_PCT:
> +    {
> +        struct cpufreq_policy new_policy;
> +        struct perf_limits *limits = &new_policy.limits;
> +
> +        new_policy = *policy;
> +        limits->min_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
> +			limits->min_policy_pct, limits->max_policy_pct);
> +        ret = __cpufreq_set_policy(policy, &new_policy);
> +        break;
> +    }

Shouldn't both of them bail when !internal_gov?

Also - do no other of the sub-ops need adjustment for the case
when an internal governor is in use?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -315,8 +315,18 @@ struct xen_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;

scaling_min and scaling_max should really be of the same type, so
that someone wanting to introduce helper functions or pointers to
them can hand both interchangeably.

Also I'm starting to get tired of repeating that it is still unclear how
a consumer of the structure will know which of the two fields of the
unions are applicable.

Jan