| Message ID | 1308911742-27394-3-git-send-email-premi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: > Currently, loops_per_jiffy is being calculated before calling > cpufreq_notify_transition(). > > However, generic cpufreq driver adjusts the jiffies as well. > Double adjustment leads to incorrect value being assigned to > loops_per_jiffy. Are you sure the generic cpufreq driver adjusts the per-cpu loops_per_jiffy values? I don't believe it does. > diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c > index ce9d534..346519e 100644 > --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c > +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c > @@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy, > > freqs.new = omap_getspeed(policy->cpu); > > - /* > - * In the generic cpufreq driver jiffies are updated only for > - * non-SMP cases. Ensure that jiffies are bing updated for both > - * SMP systems and UP systems built with CONFIG_SMP enabled. > - */ > + /* Notify transitions */ > if (is_smp()) { > -#ifdef CONFIG_SMP > - for_each_cpu(i, policy->cpus) > - per_cpu(cpu_data, i).loops_per_jiffy = > - cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy, > - freqs.old, > - freqs.new); > -#endif > - /* Notify transitions */ So this is a NAK. What's also missing is a scaling of loops_per_jiffy itself here, because with SMP=y the core won't do this for you. > for_each_cpu(i, policy->cpus) { > freqs.cpu = i; > cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); > } > } else { > - loops_per_jiffy = cpufreq_scale(loops_per_jiffy, > - freqs.old, freqs.new); > - > - /* Notify transitions */ This is almost right - the core cpufreq code looks after this when CONFIG_SMP is not selected. However, if you're running a kernel built for SMP on UP, then loops_per_jiffy won't be scaled, so something needs to be done to cover that case. Note also that you should scale the loops_per_jiffy against a reference value, otherwise you'll get an increasing error each time you scale. So, if we want to do this then we need to store the boot-time lpj and frequency as the baseline reference, and scale according to that, much like the core cpufreq code does for loops_per_jiffy for the !SMP case.
> -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > Sent: Friday, June 24, 2011 4:14 PM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > loops_per_jiffy calculation > > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: > > Currently, loops_per_jiffy is being calculated before calling > > cpufreq_notify_transition(). > > > > However, generic cpufreq driver adjusts the jiffies as well. > > Double adjustment leads to incorrect value being assigned to > > loops_per_jiffy. > > Are you sure the generic cpufreq driver adjusts the per-cpu > loops_per_jiffy > values? I don't believe it does. Russell, I am quoting the function from drivers/cpufreq/cpufreq.h Follow the arrows: [quote] void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state) { [snip]...[snip] case CPUFREQ_PRECHANGE: [snip]...[snip] srcu_notifier_call_chain(&cpufreq_transition_notifier_list, CPUFREQ_PRECHANGE, freqs); ----> adjust_jiffies(CPUFREQ_PRECHANGE, freqs); break; case CPUFREQ_POSTCHANGE: ----> adjust_jiffies(CPUFREQ_POSTCHANGE, freqs); [snip]...[snip] } } [/quote] Same can be viewed here: http://lxr.free-electrons.com/source/drivers/cpufreq/cpufreq.c?a=arm#L300 ~sanjeev [snip]...[snip]
On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote: > > -----Original Message----- > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > > Sent: Friday, June 24, 2011 4:14 PM > > To: Premi, Sanjeev > > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > > loops_per_jiffy calculation > > > > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: > > > Currently, loops_per_jiffy is being calculated before calling > > > cpufreq_notify_transition(). > > > > > > However, generic cpufreq driver adjusts the jiffies as well. > > > Double adjustment leads to incorrect value being assigned to > > > loops_per_jiffy. > > > > Are you sure the generic cpufreq driver adjusts the per-cpu > > loops_per_jiffy > > values? I don't believe it does. > > Russell, > > I am quoting the function from drivers/cpufreq/cpufreq.h > Follow the arrows: Let's go to war with quotes then, because clearly you haven't read the code properly: | #ifndef CONFIG_SMP ^^^^^^^^^^^^^^^^^^^^^ | static unsigned long l_p_j_ref; | static unsigned int l_p_j_ref_freq; | | static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) | { | ... code to adjust jiffies ... | } | #else | static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) | { | return; | } | #endif Notice how if CONFIG_SMP is set, adjust_jiffies becomes a no-op. So if CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
> -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > Sent: Friday, June 24, 2011 4:22 PM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > loops_per_jiffy calculation > > On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote: > > > -----Original Message----- > > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > > > Sent: Friday, June 24, 2011 4:14 PM > > > To: Premi, Sanjeev > > > Cc: linux-omap@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > > > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > > > loops_per_jiffy calculation > > > > > > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: > > > > Currently, loops_per_jiffy is being calculated before calling > > > > cpufreq_notify_transition(). > > > > > > > > However, generic cpufreq driver adjusts the jiffies as well. > > > > Double adjustment leads to incorrect value being assigned to > > > > loops_per_jiffy. > > > > > > Are you sure the generic cpufreq driver adjusts the per-cpu > > > loops_per_jiffy > > > values? I don't believe it does. > > > > Russell, > > > > I am quoting the function from drivers/cpufreq/cpufreq.h > > Follow the arrows: > > Let's go to war with quotes then, because clearly you haven't > read the code > properly: [sp] Peace. No war! Looks like I missed the ifndef in systax highlighting on my editor. As mentioned, I couldn't test on OMAP4. The patch obvioulsy works for A8... SMP needs to be fixed. v2 coming soon. ~sanjeev > > | #ifndef CONFIG_SMP > ^^^^^^^^^^^^^^^^^^^^^ > | static unsigned long l_p_j_ref; > | static unsigned int l_p_j_ref_freq; > | > | static void adjust_jiffies(unsigned long val, struct > cpufreq_freqs *ci) > | { > | ... code to adjust jiffies ... > | } > | #else > | static inline void adjust_jiffies(unsigned long val, struct > cpufreq_freqs *ci) > | { > | return; > | } > | #endif > > Notice how if CONFIG_SMP is set, adjust_jiffies becomes a > no-op. So if > CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code. >
Sanjeev, On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote: > On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote: >>> -----Original Message----- >>> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] >>> Sent: Friday, June 24, 2011 4:14 PM >>> To: Premi, Sanjeev >>> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org >>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix >>> loops_per_jiffy calculation >>> >>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: >>>> Currently, loops_per_jiffy is being calculated before calling >>>> cpufreq_notify_transition(). >>>> >>>> However, generic cpufreq driver adjusts the jiffies as well. >>>> Double adjustment leads to incorrect value being assigned to >>>> loops_per_jiffy. >>> >>> Are you sure the generic cpufreq driver adjusts the per-cpu >>> loops_per_jiffy >>> values? I don't believe it does. >> >> Russell, >> >> I am quoting the function from drivers/cpufreq/cpufreq.h >> Follow the arrows: > > Let's go to war with quotes then, because clearly you haven't read the code > properly: > > | #ifndef CONFIG_SMP > ^^^^^^^^^^^^^^^^^^^^^ > | static unsigned long l_p_j_ref; > | static unsigned int l_p_j_ref_freq; > | > | static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) > | { > | ... code to adjust jiffies ... > | } > | #else > | static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) > | { > | return; > | } > | #endif > > Notice how if CONFIG_SMP is set, adjust_jiffies becomes a no-op. So if > CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code. > As Russell rightly pointed out, you need to take care of UP/SMP and UP OVER SMP. The generic code updates in only in case of UP build. I thought, the comment is the code was well explaining that part. Regards Santosh
> -----Original Message----- > From: Shilimkar, Santosh > Sent: Friday, June 24, 2011 6:16 PM > To: Russell King - ARM Linux > Cc: Premi, Sanjeev; linux-omap@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > loops_per_jiffy calculation > > Sanjeev, > > On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote: > > On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote: > >>> -----Original Message----- > >>> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > >>> Sent: Friday, June 24, 2011 4:14 PM > >>> To: Premi, Sanjeev > >>> Cc: linux-omap@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > >>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix > >>> loops_per_jiffy calculation > >>> > >>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: > >>>> Currently, loops_per_jiffy is being calculated before calling > >>>> cpufreq_notify_transition(). > >>>> > >>>> However, generic cpufreq driver adjusts the jiffies as well. > >>>> Double adjustment leads to incorrect value being assigned to > >>>> loops_per_jiffy. > >>> > >>> Are you sure the generic cpufreq driver adjusts the per-cpu > >>> loops_per_jiffy > >>> values? I don't believe it does. > >> > >> Russell, > >> > >> I am quoting the function from drivers/cpufreq/cpufreq.h > >> Follow the arrows: > > > > Let's go to war with quotes then, because clearly you > haven't read the code > > properly: > > > > | #ifndef CONFIG_SMP > > ^^^^^^^^^^^^^^^^^^^^^ > > | static unsigned long l_p_j_ref; > > | static unsigned int l_p_j_ref_freq; > > | > > | static void adjust_jiffies(unsigned long val, struct > cpufreq_freqs *ci) > > | { > > | ... code to adjust jiffies ... > > | } > > | #else > > | static inline void adjust_jiffies(unsigned long val, > struct cpufreq_freqs *ci) > > | { > > | return; > > | } > > | #endif > > > > Notice how if CONFIG_SMP is set, adjust_jiffies becomes a > no-op. So if > > CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code. > > > As Russell rightly pointed out, you need to take care of > UP/SMP and UP > OVER SMP. > > The generic code updates in only in case of UP build. I thought, the > comment is the code was well explaining that part. [sp] I did read the code but took long to understand most of it. Hence patch 1 of this series. I already accepted that I didn't notice definition of adjust_jiffies. ...call it perils of leaving job half-done across a good-night-sleep! As is, the calculations of UP are incorrect... which I was fixing. Anyway, I have an updated patch - currently testing with CONFIG_SMP enabled. Will be posting soon. ~sanjeev > > Regards > Santosh >
On 6/24/2011 6:22 PM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Shilimkar, Santosh >> Sent: Friday, June 24, 2011 6:16 PM >> To: Russell King - ARM Linux >> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix >> loops_per_jiffy calculation >> >> Sanjeev, >> >> On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote: >>> On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote: >>>>> -----Original Message----- >>>>> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] >>>>> Sent: Friday, June 24, 2011 4:14 PM >>>>> To: Premi, Sanjeev >>>>> Cc: linux-omap@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org >>>>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix >>>>> loops_per_jiffy calculation >>>>> >>>>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote: >>>>>> Currently, loops_per_jiffy is being calculated before calling >>>>>> cpufreq_notify_transition(). >>>>>> >>>>>> However, generic cpufreq driver adjusts the jiffies as well. >>>>>> Double adjustment leads to incorrect value being assigned to >>>>>> loops_per_jiffy. >>>>> >>>>> Are you sure the generic cpufreq driver adjusts the per-cpu >>>>> loops_per_jiffy >>>>> values? I don't believe it does. >>>> >>>> Russell, >>>> >>>> I am quoting the function from drivers/cpufreq/cpufreq.h >>>> Follow the arrows: >>> >>> Let's go to war with quotes then, because clearly you >> haven't read the code >>> properly: >>> >>> | #ifndef CONFIG_SMP >>> ^^^^^^^^^^^^^^^^^^^^^ >>> | static unsigned long l_p_j_ref; >>> | static unsigned int l_p_j_ref_freq; >>> | >>> | static void adjust_jiffies(unsigned long val, struct >> cpufreq_freqs *ci) >>> | { >>> | ... code to adjust jiffies ... >>> | } >>> | #else >>> | static inline void adjust_jiffies(unsigned long val, >> struct cpufreq_freqs *ci) >>> | { >>> | return; >>> | } >>> | #endif >>> >>> Notice how if CONFIG_SMP is set, adjust_jiffies becomes a >> no-op. So if >>> CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code. >>> >> As Russell rightly pointed out, you need to take care of >> UP/SMP and UP >> OVER SMP. >> >> The generic code updates in only in case of UP build. I thought, the >> comment is the code was well explaining that part. > > [sp] I did read the code but took long to understand most of it. > Hence patch 1 of this series. > > I already accepted that I didn't notice definition of adjust_jiffies. > ...call it perils of leaving job half-done across a good-night-sleep! > > As is, the calculations of UP are incorrect... which I was fixing. > Anyway, I have an updated patch - currently testing with CONFIG_SMP > enabled. Will be posting soon. > I can imagine what you are gonna post based on Russell comment, but will wait for your patch before commenting. Regards Santosh
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c index ce9d534..346519e 100644 --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c @@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy, freqs.new = omap_getspeed(policy->cpu); - /* - * In the generic cpufreq driver jiffies are updated only for - * non-SMP cases. Ensure that jiffies are bing updated for both - * SMP systems and UP systems built with CONFIG_SMP enabled. - */ + /* Notify transitions */ if (is_smp()) { -#ifdef CONFIG_SMP - for_each_cpu(i, policy->cpus) - per_cpu(cpu_data, i).loops_per_jiffy = - cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy, - freqs.old, - freqs.new); -#endif - /* Notify transitions */ for_each_cpu(i, policy->cpus) { freqs.cpu = i; cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); } } else { - loops_per_jiffy = cpufreq_scale(loops_per_jiffy, - freqs.old, freqs.new); - - /* Notify transitions */ cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); }
Currently, loops_per_jiffy is being calculated before calling cpufreq_notify_transition(). However, generic cpufreq driver adjusts the jiffies as well. Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in "cat /proc/cpuinfo' Signed-off-by: Sanjeev Premi <premi@ti.com> --- arch/arm/mach-omap2/omap2plus-cpufreq.c | 18 +----------------- 1 files changed, 1 insertions(+), 17 deletions(-)