From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 04/11] x86/intel_pstate: relocate the driver register function Date: Tue, 23 Jun 2015 08:31:12 +0100 Message-ID: <558927600200007800087EFA@mail.emea.novell.com> References: <1434011230-17052-1-git-send-email-wei.w.wang@intel.com> <5582F2140200007800086ADF@mail.emea.novell.com> <286AC319A985734F985F78AFA26841F7996BB9@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <286AC319A985734F985F78AFA26841F7996BB9@shsmsx102.ccr.corp.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei W Wang Cc: "andrew.cooper3@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org >>> On 23.06.15 at 05:40, wrote: > On 18/06/2015 22:30, Jan Beulich wrote: >> >>> On 11.06.15 at 10:27, wrote: >> > Register the CPU hotplug notifier when the driver is registered, and >> > move the driver register function to the cpufreq.c. >> >> The first half of the sentence fails to say why. And I suppose if you > explained >> that (to yourself) you'd figure that the change is wrong (or at least > altering >> behavior in a way that needs more explanation to be verifiably correct): The >> calls to cpufreq_register_driver() sit in __initcall handlers, yet what you >> replace is a presmp_initcall. I.e. all APs being brought up at boot time won't >> get the callback invoked for them anymore. >> >> I suppose you tested your series on a system where the new driver will kick >> in. You of course also need to test the case where this isn't the case - >> everything needs to continue to function there. > > The cpu_callback() is removed in the following patch (05/11) because it's > redundant. > Functions in cpufreq_register_driver() are also called only once, because > other calls to cpufreq_register_driver() will just return due to the > unsuccessful condition check at the beginning of the function. If this is the case today, then the removal should be a separate patch early in the series (properly explaining the situation). >> > --- a/xen/drivers/cpufreq/cpufreq.c >> > +++ b/xen/drivers/cpufreq/cpufreq.c >> > @@ -630,12 +630,21 @@ static struct notifier_block cpu_nfb = { >> > .notifier_call = cpu_callback >> > }; >> > >> > -static int __init cpufreq_presmp_init(void) >> > +int cpufreq_register_driver(struct cpufreq_driver *driver_data) >> > { >> > void *cpu = (void *)(long)smp_processor_id(); >> > cpu_callback(&cpu_nfb, CPU_ONLINE, cpu); >> > + if (!driver_data || !driver_data->init >> > + || !driver_data->verify || !driver_data->exit >> > + || (!driver_data->target == !driver_data->setpolicy)) >> >> Do you really want/need to enforce this policy (target set if and only if >> setpolicy is not set) here? And if that's to uniformly hold, the two could be >> put into a union... > > driver_data->target() is used by a driver which relies on the old Governor > framework. > driver_data->setpolicy() is used by a driver which implements its internal > governor. > So, the driver either uses the old Governor framework or has its own private > internal governor. > We shouldn't change to use union, because in many places, we distinguish the > two by checking if it's using "->target" or "->setpolicy". The distinction between the two driver modes shouldn't be based on arbitrary accessors they may or may not implement. There should be a dedicated flag or alike. Jan