Re: [Xen-devel] [PATCH v5 5/9] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.15 at 04:32, <wei.w.wang@xxxxxxxxx> wrote:
> We change to NULL the cpufreq_cpu_policy pointer after the call of
> cpufreq_driver->exit, because the pointer is still needed in
> intel_pstate_set_pstate().
> 
> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx>
> ---
>  xen/drivers/cpufreq/cpufreq.c      | 6 +++---
>  xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
>  changes in v5:
>  1) put this patch prior to the "main body of intel pstate driver", which is 
>     one of the acceptable options suggested by the Jan.

Did I? With the description above, I still don't see why this would be
needed (here or later):

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -334,12 +334,11 @@ 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);
>  
>      cpufreq_statistic_exit(cpu);
> -    per_cpu(cpufreq_cpu_policy, cpu) = NULL;

Prior to this line we still have policy == per_cpu(cpufreq_cpu_policy, cpu),
i.e. the data needed is being made available to the ->exit() handler
called a few lines down. I.e. if you want to convince me or anyone
else that the change is needed, you will need to say in the description
why that is not enough.

> --- 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;
> +};
> +
>  struct cpufreq_policy {
>      cpumask_var_t       cpus;          /* affected CPUs */
>      unsigned int        shared_type;   /* ANY or ALL affected CPUs
> @@ -66,6 +72,7 @@ struct cpufreq_policy {
>                                   * governors are used */
>      struct perf_limits  limits;
>      struct cpufreq_governor     *governor;
> +    struct internal_governor    *internal_gov;

Both of the changes to this file look unrelated to the purpose of the
patch, as does the one consumer of the new pointer.

Jan


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