[PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Wei Wang]

cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
to NULL it after the call of cpufreq_driver->exit. Otherwise, a
calltrace will show up on your screen due to the reference of a NULL
pointer when you power down the system.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 xen/drivers/cpufreq/cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 6003a8c..a8772e8 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -335,12 +335,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->policy && (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;
     cpumask_clear_cpu(cpu, policy->cpus);
     cpumask_clear_cpu(cpu, cpufreq_dom->map);
 
@@ -349,6 +348,7 @@ int cpufreq_del_cpu(unsigned int cpu)
         free_cpumask_var(policy->cpus);
         xfree(policy);
     }
+    per_cpu(cpufreq_cpu_policy, cpu) = NULL;
 
     /* for the last cpu of the domain, clean room */
     /* It's safe here to free freq_table, drv_data and policy */
-- 
1.9.1

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Julien Grall]

Hi,

On 11/06/2015 04:28, Wei Wang wrote:
> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
> to NULL it after the call of cpufreq_driver->exit. Otherwise, a
> calltrace will show up on your screen due to the reference of a NULL
> pointer when you power down the system.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   xen/drivers/cpufreq/cpufreq.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
> index 6003a8c..a8772e8 100644
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -335,12 +335,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->policy && (hw_all || (cpumask_weight(cpufreq_dom->map) ==
> +                   perf->domain_info.num_processors)))

Based on your patch #6, the field policy contains value which is defined 
per-cpufreq driver (because you defined internal value). How can you be 
sure that a driver will never use 0 as a valid value?

Regards,

-- 
Julien Grall

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Wang, Wei W]

On 11/06/2015 22:06, Julien Grall wrote:
> On 11/06/2015 04:28, Wei Wang wrote:
> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
> > to NULL it after the call of cpufreq_driver->exit. Otherwise, a
> > calltrace will show up on your screen due to the reference of a NULL
> > pointer when you power down the system.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > ---
> >   xen/drivers/cpufreq/cpufreq.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/drivers/cpufreq/cpufreq.c
> > b/xen/drivers/cpufreq/cpufreq.c index 6003a8c..a8772e8 100644
> > --- a/xen/drivers/cpufreq/cpufreq.c
> > +++ b/xen/drivers/cpufreq/cpufreq.c
> > @@ -335,12 +335,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->policy && (hw_all || (cpumask_weight(cpufreq_dom->map)
> ==
> > +                   perf->domain_info.num_processors)))
> 
> Based on your patch #6, the field policy contains value which is defined per-
> cpufreq driver (because you defined internal value). How can you be sure
> that a driver will never use 0 as a valid value?

Hi Julien, what do you mean by "per-cpufreq driver"? 

We currently have two P-state drivers. This filed is currently only used by the intel_pstate driver, and the four usable values are:
#define CPUFREQ_POLICY_POWERSAVE           (1)
#define CPUFREQ_POLICY_PERFORMANCE      (2)
#define CPUFREQ_POLICY_USERSPACE              (3)
#define CPUFREQ_POLICY_ONDEMAND            (4)

The intel_pstate won't use 0 as a valid value, and the default value is CPUFREQ_POLICY_ONDEMAND.  If it's 0, it basically means the old acpi-cpufreq driver is being used.

Best,
Wei

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Julien Grall]

On 11/06/2015 22:01, Wang, Wei W wrote:
> On 11/06/2015 22:06, Julien Grall wrote:
>> On 11/06/2015 04:28, Wei Wang wrote:
>>> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
>>> to NULL it after the call of cpufreq_driver->exit. Otherwise, a
>>> calltrace will show up on your screen due to the reference of a NULL
>>> pointer when you power down the system.
>>>
>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> ---
>>>    xen/drivers/cpufreq/cpufreq.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/drivers/cpufreq/cpufreq.c
>>> b/xen/drivers/cpufreq/cpufreq.c index 6003a8c..a8772e8 100644
>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> @@ -335,12 +335,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->policy && (hw_all || (cpumask_weight(cpufreq_dom->map)
>> ==
>>> +                   perf->domain_info.num_processors)))
>>
>> Based on your patch #6, the field policy contains value which is defined per-
>> cpufreq driver (because you defined internal value). How can you be sure
>> that a driver will never use 0 as a valid value?
>
> Hi Julien, what do you mean by "per-cpufreq driver"?
>
> We currently have two P-state drivers. This filed is currently only used by the intel_pstate driver, and the four usable values are:
> #define CPUFREQ_POLICY_POWERSAVE           (1)
> #define CPUFREQ_POLICY_PERFORMANCE      (2)
> #define CPUFREQ_POLICY_USERSPACE              (3)
> #define CPUFREQ_POLICY_ONDEMAND            (4)
>
> The intel_pstate won't use 0 as a valid value, and the default value is CPUFREQ_POLICY_ONDEMAND.  If it's 0, it basically means the old acpi-cpufreq driver is being used.

You seem to rely on nobody else with use the cpufreq framework... which 
is wrong. intel_pstate won't be the only possible cpufreq driver. Some 
ARM developper are working on adding a cpufreq for ARM power management.

You said that CPUFREQ_POLICY_* is specific to the intel driver. But use 
them in the common code. If any cpufreq driver can use the value, then 
make it common. Otherwise please move this code outside of the framework.

Regards,

-- 
Julien Grall

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Wang, Wei W]

On 12/06/2015 19:40, Julien Grall wrote:
> On 11/06/2015 22:01, Wang, Wei W wrote:
> > On 11/06/2015 22:06, Julien Grall wrote:
> >> On 11/06/2015 04:28, Wei Wang wrote:
> >>> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we
> >>> change to NULL it after the call of cpufreq_driver->exit. Otherwise,
> >>> a calltrace will show up on your screen due to the reference of a
> >>> NULL pointer when you power down the system.
> >>>
> >>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> >>> ---
> >>>    xen/drivers/cpufreq/cpufreq.c | 6 +++---
> >>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/drivers/cpufreq/cpufreq.c
> >>> b/xen/drivers/cpufreq/cpufreq.c index 6003a8c..a8772e8 100644
> >>> --- a/xen/drivers/cpufreq/cpufreq.c
> >>> +++ b/xen/drivers/cpufreq/cpufreq.c
> >>> @@ -335,12 +335,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->policy && (hw_all ||
> >>> + (cpumask_weight(cpufreq_dom->map)
> >> ==
> >>> +                   perf->domain_info.num_processors)))
> >>
> >> Based on your patch #6, the field policy contains value which is
> >> defined per- cpufreq driver (because you defined internal value). How
> >> can you be sure that a driver will never use 0 as a valid value?
> >
> > Hi Julien, what do you mean by "per-cpufreq driver"?
> >
> > We currently have two P-state drivers. This filed is currently only used by
> the intel_pstate driver, and the four usable values are:
> > #define CPUFREQ_POLICY_POWERSAVE           (1)
> > #define CPUFREQ_POLICY_PERFORMANCE      (2)
> > #define CPUFREQ_POLICY_USERSPACE              (3)
> > #define CPUFREQ_POLICY_ONDEMAND            (4)
> >
> > The intel_pstate won't use 0 as a valid value, and the default value is
> CPUFREQ_POLICY_ONDEMAND.  If it's 0, it basically means the old acpi-
> cpufreq driver is being used.
> 
> You seem to rely on nobody else with use the cpufreq framework... which is
> wrong. intel_pstate won't be the only possible cpufreq driver. Some ARM
> developper are working on adding a cpufreq for ARM power management.
> 
> You said that CPUFREQ_POLICY_* is specific to the intel driver. But use them
> in the common code. If any cpufreq driver can use the value, then make it
> common. Otherwise please move this code outside of the framework.

To me, we can deal with your concerns by
1) renaming the previously mentioned things to be intel_pstate specific;
2) moving them to a new header file.

Jan, please also ACK if you agree on these changes.

Best,
Wei

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Jan Beulich]

>>> On 15.06.15 at 02:31, <wei.w.wang@intel.com> wrote:
> On 12/06/2015 19:40, Julien Grall wrote:
>> On 11/06/2015 22:01, Wang, Wei W wrote:
>> > On 11/06/2015 22:06, Julien Grall wrote:
>> >> On 11/06/2015 04:28, Wei Wang wrote:
>> >>> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we
>> >>> change to NULL it after the call of cpufreq_driver->exit. Otherwise,
>> >>> a calltrace will show up on your screen due to the reference of a
>> >>> NULL pointer when you power down the system.
>> >>>
>> >>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> >>> ---
>> >>>    xen/drivers/cpufreq/cpufreq.c | 6 +++---
>> >>>    1 file changed, 3 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/xen/drivers/cpufreq/cpufreq.c
>> >>> b/xen/drivers/cpufreq/cpufreq.c index 6003a8c..a8772e8 100644
>> >>> --- a/xen/drivers/cpufreq/cpufreq.c
>> >>> +++ b/xen/drivers/cpufreq/cpufreq.c
>> >>> @@ -335,12 +335,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->policy && (hw_all ||
>> >>> + (cpumask_weight(cpufreq_dom->map)
>> >> ==
>> >>> +                   perf->domain_info.num_processors)))
>> >>
>> >> Based on your patch #6, the field policy contains value which is
>> >> defined per- cpufreq driver (because you defined internal value). How
>> >> can you be sure that a driver will never use 0 as a valid value?
>> >
>> > Hi Julien, what do you mean by "per-cpufreq driver"?
>> >
>> > We currently have two P-state drivers. This filed is currently only used by
>> the intel_pstate driver, and the four usable values are:
>> > #define CPUFREQ_POLICY_POWERSAVE           (1)
>> > #define CPUFREQ_POLICY_PERFORMANCE      (2)
>> > #define CPUFREQ_POLICY_USERSPACE              (3)
>> > #define CPUFREQ_POLICY_ONDEMAND            (4)
>> >
>> > The intel_pstate won't use 0 as a valid value, and the default value is
>> CPUFREQ_POLICY_ONDEMAND.  If it's 0, it basically means the old acpi-
>> cpufreq driver is being used.
>> 
>> You seem to rely on nobody else with use the cpufreq framework... which is
>> wrong. intel_pstate won't be the only possible cpufreq driver. Some ARM
>> developper are working on adding a cpufreq for ARM power management.
>> 
>> You said that CPUFREQ_POLICY_* is specific to the intel driver. But use them
>> in the common code. If any cpufreq driver can use the value, then make it
>> common. Otherwise please move this code outside of the framework.
> 
> To me, we can deal with your concerns by
> 1) renaming the previously mentioned things to be intel_pstate specific;
> 2) moving them to a new header file.
> 
> Jan, please also ACK if you agree on these changes.

Your proposal doesn't address the use of the constants in common
code, as observed by Julien.

Jan

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Jan Beulich]

>>> On 11.06.15 at 10:28, <wei.w.wang@intel.com> wrote:
> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
> to NULL it after the call of cpufreq_driver->exit. Otherwise, a
> calltrace will show up on your screen due to the reference of a NULL
> pointer when you power down the system.

Apart from what was already said on this patch, I think it is placed
too late in this series (should precede patch 6) or, even better, not
needed at all: ->exit() gets passed the policy being cleaned up, so
I don't follow why the driver needs to consult the per-CPU field to
obtain it.

Plus, if the patch is to be kept, the description suggesting this to be
a problem at system shutdown only is wrong - it can equally well
happen while offlining a CPU. Just saying that the pointer is still
needed would be sufficient.

Jan

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Wang, Wei W]

On 19/06/2015 17:44, Jan Beulich wrote:
> >>> On 11.06.15 at 10:28, <wei.w.wang@intel.com> wrote:
> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
> > to NULL it after the call of cpufreq_driver->exit. Otherwise, a
> > calltrace will show up on your screen due to the reference of a NULL
> > pointer when you power down the system.
> 
> Apart from what was already said on this patch, I think it is placed too late in
> this series (should precede patch 6) or, even better, not needed at all: ->exit()
> gets passed the policy being cleaned up, so I don't follow why the driver
> needs to consult the per-CPU field to obtain it.

> Plus, if the patch is to be kept, the description suggesting this to be a
> problem at system shutdown only is wrong - it can equally well happen while
> offlining a CPU. Just saying that the pointer is still needed would be sufficient.

It's needed. When unplugging a CPU, the intel_pstate driver sets it to run with the lowest P-state.

Best,
Wei

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Jan Beulich]

>>> On 23.06.15 at 08:16, <wei.w.wang@intel.com> wrote:
> On 19/06/2015 17:44, Jan Beulich wrote:
>> >>> On 11.06.15 at 10:28, <wei.w.wang@intel.com> wrote:
>> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we change
>> > to NULL it after the call of cpufreq_driver->exit. Otherwise, a
>> > calltrace will show up on your screen due to the reference of a NULL
>> > pointer when you power down the system.
>> 
>> Apart from what was already said on this patch, I think it is placed too late in
>> this series (should precede patch 6) or, even better, not needed at all: ->exit()
>> gets passed the policy being cleaned up, so I don't follow why the driver
>> needs to consult the per-CPU field to obtain it.
> 
>> Plus, if the patch is to be kept, the description suggesting this to be a
>> problem at system shutdown only is wrong - it can equally well happen while
>> offlining a CPU. Just saying that the pointer is still needed would be sufficient.
> 
> It's needed. When unplugging a CPU, the intel_pstate driver sets it to run 
> with the lowest P-state.

I understand the latter, but this doesn't explain why you can't do
what I suggested above.

Jan

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Wang, Wei W]

On 23/06/2015 16:08, Jan Beulich wrote:
> >>> On 23.06.15 at 08:16, <wei.w.wang@intel.com> wrote:
> > On 19/06/2015 17:44, Jan Beulich wrote:
> >> >>> On 11.06.15 at 10:28, <wei.w.wang@intel.com> wrote:
> >> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we
> >> > change to NULL it after the call of cpufreq_driver->exit.
> >> > Otherwise, a calltrace will show up on your screen due to the
> >> > reference of a NULL pointer when you power down the system.
> >>
> >> Apart from what was already said on this patch, I think it is placed
> >> too late in this series (should precede patch 6) or, even better, not
> >> needed at all: ->exit() gets passed the policy being cleaned up, so I
> >> don't follow why the driver needs to consult the per-CPU field to obtain it.
> >
> >> Plus, if the patch is to be kept, the description suggesting this to
> >> be a problem at system shutdown only is wrong - it can equally well
> >> happen while offlining a CPU. Just saying that the pointer is still needed
> would be sufficient.
> >
> > It's needed. When unplugging a CPU, the intel_pstate driver sets it to
> > run with the lowest P-state.
> 
> I understand the latter, but this doesn't explain why you can't do what I
> suggested above.

Because the "->exit()" needs to call "intel_pstate_set_pstate()" which does not receive "policy" as a parameter. "intel_pstate_set_pstate()" is also called by another function without passing "policy". So I think it is simpler to just change the order of NULLing the pointer, instead of changing more code.

Best,
Wei  

Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

[Jan Beulich]

>>> On 23.06.15 at 10:21, <wei.w.wang@intel.com> wrote:
> On 23/06/2015 16:08, Jan Beulich wrote:
>> >>> On 23.06.15 at 08:16, <wei.w.wang@intel.com> wrote:
>> > On 19/06/2015 17:44, Jan Beulich wrote:
>> >> >>> On 11.06.15 at 10:28, <wei.w.wang@intel.com> wrote:
>> >> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we
>> >> > change to NULL it after the call of cpufreq_driver->exit.
>> >> > Otherwise, a calltrace will show up on your screen due to the
>> >> > reference of a NULL pointer when you power down the system.
>> >>
>> >> Apart from what was already said on this patch, I think it is placed
>> >> too late in this series (should precede patch 6) or, even better, not
>> >> needed at all: ->exit() gets passed the policy being cleaned up, so I
>> >> don't follow why the driver needs to consult the per-CPU field to obtain it.
>> >
>> >> Plus, if the patch is to be kept, the description suggesting this to
>> >> be a problem at system shutdown only is wrong - it can equally well
>> >> happen while offlining a CPU. Just saying that the pointer is still needed
>> would be sufficient.
>> >
>> > It's needed. When unplugging a CPU, the intel_pstate driver sets it to
>> > run with the lowest P-state.
>> 
>> I understand the latter, but this doesn't explain why you can't do what I
>> suggested above.
> 
> Because the "->exit()" needs to call "intel_pstate_set_pstate()" which does 
> not receive "policy" as a parameter. "intel_pstate_set_pstate()" is also 
> called by another function without passing "policy". So I think it is simpler 
> to just change the order of NULLing the pointer, instead of changing more 
> code.

I yet have to see the amount of differences to the Linux original to be
convinced that this is the better approach, so let's see how the next
version of the series is going to look like.

Jan