Re: [Xen-devel] [PATCH v6 2/6] x86/intel_pstate: introduce the internal_governor struct

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 04:21, <wei.w.wang@xxxxxxxxx> wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -334,8 +334,9 @@ int cpufreq_del_cpu(unsigned int cpu)
>  
>      /* for HW_ALL, stop gov for each core of the _PSD domain */
>      /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */
> -    if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
> -                   perf->domain_info.num_processors))
> +    if (!policy->internal_gov &&
> +        (hw_all || (cpumask_weight(cpufreq_dom->map) ==
> +                    perf->domain_info.num_processors)))
>          __cpufreq_governor(policy, CPUFREQ_GOV_STOP);

Why do we need such a guard here, but not on any of the other
__cpufreq_governor() invocations? (If they will end up
unreachable by the following patches, then I'd still expect one
or more ASSERT()s to be added to document/enforce this.)

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -53,6 +53,12 @@ struct perf_limits {
>      uint32_t min_policy_pct;
>  };
>  
> +struct internal_governor {
> +    char *avail_gov;
> +    uint32_t gov_num;
> +    uint32_t cur_gov;
> +};

Apart from the fields being meaningless without user I don't think
the gov_ prefix and the _gov suffixes are really helpful here.
Also "num" isn't really unambiguous - if you mean "count", please
use "count". If you mean something else, please pick an
unambiguous field name.

Jan


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