[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |