Re: [Xen-devel] [PATCH v2 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 09:51, <wei.w.wang@xxxxxxxxx> wrote:
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -167,7 +167,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
>   * 2. Provide user PM control
>   */
>  static int read_scaling_available_governors(char 
> *scaling_available_governors,
> -                                            unsigned int size)
> +                              unsigned int size, unsigned int is_internal)

Please don't break indentation aiming at suitable alignment. And
is_internal looks like it wants to be bool_t (and probably doesn't
need the is_ prefix).

> @@ -175,12 +175,19 @@ static int read_scaling_available_governors(char 
> *scaling_available_governors,
>      if ( !scaling_available_governors )
>          return -EINVAL;
>  
> -    list_for_each_entry(t, &cpufreq_governor_list, governor_list)
> -    {
> -        i += scnprintf(&scaling_available_governors[i],
> -                       CPUFREQ_NAME_LEN, "%s ", t->name);
> -        if ( i > size )
> -            return -EINVAL;
> +    if (is_internal) {

Coding style (throughout the changes to this file).

> +        i += scnprintf(&scaling_available_governors[0], CPUFREQ_NAME_LEN, 
> "%s ", "performance");
> +        i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, 
> "%s ", "powersave");
> +        i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, 
> "%s ", "userspace");
> +        i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, 
> "%s ", "ondemand");

Long lines.

> @@ -203,11 +210,15 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
> *op)
>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>  
>      if ( !pmpt || !pmpt->perf.states ||
> -         !policy || !policy->governor )
> +         !policy || (!policy->governor && !policy->policy) )
>          return -EINVAL;
>  
> -    list_for_each(pos, &cpufreq_governor_list)
> -        gov_num++;
> +    if (policy->policy)
> +        gov_num = 4;

This literal number again needs a #define.

> @@ -261,29 +272,47 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
>      op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
>      op->u.get_para.scaling_cur_freq = policy->cur;
> -    op->u.get_para.scaling_max_freq = policy->max;
> -    op->u.get_para.scaling_min_freq = policy->min;
> +    if (policy->policy) {
> +        op->u.get_para.scaling_max.max_perf_pct = policy->max_perf_pct;
> +        op->u.get_para.scaling_min.min_perf_pct = policy->min_perf_pct;
> +        op->u.get_para.scaling_turbo_pct = policy->turbo_pct;
> +    } else {
> +        op->u.get_para.scaling_max.max_freq = policy->max;
> +        op->u.get_para.scaling_min.min_freq = policy->min;
> +    }

How does the caller then know which of the union member meanings
apply?

> -    if ( policy->governor->name[0] )
> -        strlcpy(op->u.get_para.scaling_governor, 
> +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE)

switch() again.

>      /* governor specific para */
> -    if ( !strnicmp(op->u.get_para.scaling_governor, 
> +    if ( !strnicmp(op->u.get_para.scaling_governor,
>                     "userspace", CPUFREQ_NAME_LEN) )
>      {
>          op->u.get_para.u.userspace.scaling_setspeed = policy->cur;
>      }
>  
> -    if ( !strnicmp(op->u.get_para.scaling_governor, 
> +    if ( !strnicmp(op->u.get_para.scaling_governor,

Please can you avoid such white space adjustments to otherwise
untouched code in an already big patch?

> @@ -348,6 +392,24 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>          break;
>      }
>  
> +    case SCALING_MAX_PCT:
> +    {
> +        struct cpufreq_policy new_policy;
> +        memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));

Blank line between declarations and statements please. Also please
use structure assignment (type safe) instead of memcpy() (not type
safe), and then perhaps right as initializer for the variable.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -314,8 +314,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 max_freq;
> +        int32_t  max_perf_pct;
> +    } scaling_max;
> +
> +    union {
> +        uint32_t min_freq;
> +        int32_t  min_perf_pct;
> +    } scaling_min;
> +
> +    int32_t scaling_turbo_pct;

Again all of these should presumably be uint32_t. Plus the max/min
duplication should be avoided (i.e. drop the prefixes from the union
fields). Nor do I think perf_pct is really all that useful - pct or
percentage would do afaict.

Jan

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