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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 02:31, <wei.w.wang@xxxxxxxxx> 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@xxxxxxxxx>
>> >>> ---
>> >>>    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


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