| Message ID | 1308923618-5333-1-git-send-email-premi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Premi, Sanjeev > Sent: Friday, June 24, 2011 7:24 PM > To: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Cc: Premi, Sanjeev > Subject: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy > calculation > > Currently, loops_per_jiffy is being calculated twice for > non-SMP processors. > - Before calling cpufreq_notify_transition() > - From within cpufreq_notify_transition() > > Double adjustment leads to incorrect value being assigned to > loops_per_jiffy. This manifests as incorrect BogoMIPS in > "cat /proc/cpuinfo". > > The value of loops_per_jiffy needs to be calculated only > when CONFIG_SMP is true. It is the core change included > in this patch. > > The patch also leverages the definition of for_each_cpu() > with and without CONFIG_SMP to consolidate the mechanism > to call cpufreq_notify_transition(). > > Signed-off-by: Sanjeev Premi <premi@ti.com> > --- > Changes since v1: > * loops_per_jiffy are updated when CONFIG_SMP is true. > * leverage definition of for_each_cpu() > > Tested on OMAP3EVM with and without CONFIG_SMP. > Since the log is rather long, will be posting the log in > a follow-up mail. > [snip]...[snip] Snapshot of test log - with and without SMP included here: To ensure compile-time and run-time checks for SMP are visible, the patch was tested with this code inserted at enty of function omap_target(). #ifdef CONFIG_SMP printk (KERN_ERR "I am defined SMP!\n"); #else printk (KERN_ERR "I am not defined SMP!\n"); #endif if (is_smp()) printk (KERN_ERR "Runtime evaluating to SMP!\n"); else printk (KERN_ERR "Runtime evaluating to no SMP!\n"); With CONFIG_SMP disabled ======================== [root@OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/ [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat scaling_available_frequencies 300000 600000 800000 1000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 300000 > scaling_setspeed [ 56.278137] I am not defined SMP! [ 56.281768] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 300000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS : 298.32 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 800000 > scaling_setspeed [ 96.938049] I am not defined SMP! [ 96.941833] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS : 796.19 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 100000 > scaling_setspeed [ 110.865631] I am not defined SMP! [ 110.870025] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# echo 1000000 > scaling_setspeed [ 116.258941] I am not defined SMP! [ 116.262725] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 1000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS : 996.74 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# With CONFIG_SMP enabled ======================== [root@OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/ [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat scaling_available_frequencies 300000 600000 800000 1000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 300000 > scaling_setspeed [ 25.040496] I am defined SMP! [ 25.043884] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) processor : 0 BogoMIPS : 298.32 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 300000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 800000 > scaling_setspeed [ 46.333618] I am defined SMP! [ 46.336822] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 800000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) processor : 0 BogoMIPS : 796.19 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 600000 > scaling_setspeed [ 1344.705413] I am defined SMP! [ 1344.709259] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) processor : 0 BogoMIPS : 597.64 Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part : 0xc08 CPU revision : 2 Hardware : OMAP3 EVM Revision : 0020 Serial : 0000000000000000
On Fri, Jun 24, 2011 at 07:23:38PM +0530, Sanjeev Premi wrote: > @@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy, > return ret; > > /* Notify transitions */ > - if (is_smp()) { > - for_each_cpu(i, policy->cpus) { > - freqs.cpu = i; > - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); > - } > - } else { > + for_each_cpu(i, policy->cpus) { > + freqs.cpu = i; > cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); > } > Ok. > @@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy, > > freqs.new = omap_getspeed(policy->cpu); > > +#ifdef CONFIG_SMP > + /* Adjust jiffies before transition */ > + for_each_cpu(i, policy->cpus) { > + unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy; > + > + per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj, > + freqs.old, > + freqs.new); > + } > +#endif > + You didn't listen to what I told you. It'll be quicker for me to write this patch myself if you send me the *original* file. > /* Notify transitions */ > - if (is_smp()) { > - for_each_cpu(i, policy->cpus) { > - freqs.cpu = i; > - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); > - } > - } else { > + for_each_cpu(i, policy->cpus) { > + freqs.cpu = i; > cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); > } Ok.
> -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > Sent: Friday, June 24, 2011 7:32 PM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix > loops_per_jiffy calculation [snip]...[snip] > > > > +#ifdef CONFIG_SMP > > + /* Adjust jiffies before transition */ > > + for_each_cpu(i, policy->cpus) { > > + unsigned long lpj = per_cpu(cpu_data, > i).loops_per_jiffy; > > + > > + per_cpu(cpu_data, i).loops_per_jiffy = > cpufreq_scale(lpj, > > + freqs.old, > > + freqs.new); > > + } > > +#endif > > + > > You didn't listen to what I told you. It'll be quicker for > me to write > this patch myself if you send me the *original* file. > Russell, The function adjust_jiffies() is no-op for CONFIG_SMP - but it is not the case with cpufreq_scale(). So, when CONFIG_SMP is defined, then loops_per_jiffy would be calculated for both UP and SMP processors. Did I miss something else? ~sanjeev [snip]...[snip]
On Fri, Jun 24, 2011 at 07:39:41PM +0530, Premi, Sanjeev wrote: > > -----Original Message----- > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > > Sent: Friday, June 24, 2011 7:32 PM > > To: Premi, Sanjeev > > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix > > loops_per_jiffy calculation > > [snip]...[snip] > > > > > > > +#ifdef CONFIG_SMP > > > + /* Adjust jiffies before transition */ > > > + for_each_cpu(i, policy->cpus) { > > > + unsigned long lpj = per_cpu(cpu_data, > > i).loops_per_jiffy; > > > + > > > + per_cpu(cpu_data, i).loops_per_jiffy = > > cpufreq_scale(lpj, > > > + freqs.old, > > > + freqs.new); > > > + } > > > +#endif > > > + > > > > You didn't listen to what I told you. It'll be quicker for > > me to write > > this patch myself if you send me the *original* file. > > > > Russell, > > The function adjust_jiffies() is no-op for CONFIG_SMP - but it > is not the case with cpufreq_scale(). > > So, when CONFIG_SMP is defined, then loops_per_jiffy would be > calculated for both UP and SMP processors. > > Did I miss something else? Yes you have, and you've not sent me the file, so I can't show you where you've gone wrong.
On 6/24/2011 7:23 PM, Sanjeev Premi wrote: > Currently, loops_per_jiffy is being calculated twice for > non-SMP processors. > - Before calling cpufreq_notify_transition() > - From within cpufreq_notify_transition() > > Double adjustment leads to incorrect value being assigned to > loops_per_jiffy. This manifests as incorrect BogoMIPS in > "cat /proc/cpuinfo". > > The value of loops_per_jiffy needs to be calculated only > when CONFIG_SMP is true. It is the core change included > in this patch. > > The patch also leverages the definition of for_each_cpu() > with and without CONFIG_SMP to consolidate the mechanism > to call cpufreq_notify_transition(). > > Signed-off-by: Sanjeev Premi<premi@ti.com> NAK. This patch again doesn't make sense considering your issue. Also jiffies should not be undated before changing the freq. If the set_rate failed for some reason then you will have wrong jiffies value. I understand your issue now. The code for global lpj updation should have been checking smp_on_up() instead of is_smp(). That one line change is enough. I will post a patch on the same once I reach to office. Regards Santosh
> -----Original Message----- > From: Shilimkar, Santosh > Sent: Friday, June 24, 2011 8:05 PM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix > loops_per_jiffy calculation > > On 6/24/2011 7:23 PM, Sanjeev Premi wrote: > > Currently, loops_per_jiffy is being calculated twice for > > non-SMP processors. > > - Before calling cpufreq_notify_transition() > > - From within cpufreq_notify_transition() > > > > Double adjustment leads to incorrect value being assigned to > > loops_per_jiffy. This manifests as incorrect BogoMIPS in > > "cat /proc/cpuinfo". > > > > The value of loops_per_jiffy needs to be calculated only > > when CONFIG_SMP is true. It is the core change included > > in this patch. > > > > The patch also leverages the definition of for_each_cpu() > > with and without CONFIG_SMP to consolidate the mechanism > > to call cpufreq_notify_transition(). > > > > Signed-off-by: Sanjeev Premi<premi@ti.com> > > NAK. This patch again doesn't make sense considering your issue. > Also jiffies should not be undated before changing the freq. > If the set_rate failed for some reason then you will have wrong > jiffies value. > > I understand your issue now. The code for global lpj updation > should have been checking smp_on_up() instead of is_smp(). > That one line change is enough. I will post a patch on > the same once I reach to office. [sp] Suggest reading the patch before NAKing. The patch is updating the loops_per_jiffy value "AFTER" the frequency change has happenned and "BEFORE" post change notification is sent. ~sanjeev > > Regards > Santosh >
On 6/24/2011 7:40 AM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Shilimkar, Santosh >> Sent: Friday, June 24, 2011 8:05 PM >> To: Premi, Sanjeev >> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix >> loops_per_jiffy calculation >> >> On 6/24/2011 7:23 PM, Sanjeev Premi wrote: >>> Currently, loops_per_jiffy is being calculated twice for >>> non-SMP processors. >>> - Before calling cpufreq_notify_transition() >>> - From within cpufreq_notify_transition() >>> >>> Double adjustment leads to incorrect value being assigned to >>> loops_per_jiffy. This manifests as incorrect BogoMIPS in >>> "cat /proc/cpuinfo". >>> >>> The value of loops_per_jiffy needs to be calculated only >>> when CONFIG_SMP is true. It is the core change included >>> in this patch. >>> >>> The patch also leverages the definition of for_each_cpu() >>> with and without CONFIG_SMP to consolidate the mechanism >>> to call cpufreq_notify_transition(). >>> >>> Signed-off-by: Sanjeev Premi<premi@ti.com> >> >> NAK. This patch again doesn't make sense considering your issue. >> Also jiffies should not be undated before changing the freq. >> If the set_rate failed for some reason then you will have wrong >> jiffies value. >> >> I understand your issue now. The code for global lpj updation >> should have been checking smp_on_up() instead of is_smp(). >> That one line change is enough. I will post a patch on >> the same once I reach to office. > > [sp] Suggest reading the patch before NAKing. > The patch is updating the loops_per_jiffy value "AFTER" > the frequency change has happenned and "BEFORE" post > change notification is sent. > Sure. Let me respond to you with the patch instead of making more noise on the list. Regard Santosh
<resending as plain text> On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi <premi@ti.com> wrote: > > Currently, loops_per_jiffy is being calculated twice for > non-SMP processors. > - Before calling cpufreq_notify_transition() > - From within cpufreq_notify_transition() > > Double adjustment leads to incorrect value being assigned to > loops_per_jiffy. This manifests as incorrect BogoMIPS in > "cat /proc/cpuinfo". > > The value of loops_per_jiffy needs to be calculated only > when CONFIG_SMP is true. It is the core change included > in this patch. > > The patch also leverages the definition of for_each_cpu() > with and without CONFIG_SMP to consolidate the mechanism > to call cpufreq_notify_transition(). > > Signed-off-by: Sanjeev Premi <premi@ti.com> > --- > Changes since v1: > * loops_per_jiffy are updated when CONFIG_SMP is true. > * leverage definition of for_each_cpu() > > Tested on OMAP3EVM with and without CONFIG_SMP. > Since the log is rather long, will be posting the log in > a follow-up mail. > > arch/arm/mach-omap2/omap2plus-cpufreq.c | 27 +++++++++++++++------------ > 1 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c > index 346519e..0263cae 100644 > --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c > +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c > @@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy, > return ret; > > /* Notify transitions */ > - if (is_smp()) { > - for_each_cpu(i, policy->cpus) { > - freqs.cpu = i; > - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); > - } > - } else { > + for_each_cpu(i, policy->cpus) { > + freqs.cpu = i; > cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); > } > > @@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy, > > freqs.new = omap_getspeed(policy->cpu); > > +#ifdef CONFIG_SMP > + /* Adjust jiffies before transition */ > + for_each_cpu(i, policy->cpus) { > + unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy; > + > + per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj, > + freqs.old, > + freqs.new); Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. There were previous discussions about polling a fixed-frequency timer for udelay on SMP systems. > + } > +#endif > + > /* Notify transitions */ > - if (is_smp()) { > - for_each_cpu(i, policy->cpus) { > - freqs.cpu = i; > - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); > - } > - } else { > + for_each_cpu(i, policy->cpus) { > + freqs.cpu = i; > cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); > } > > -- > 1.7.2.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/28/2011 3:29 PM, Colin Cross wrote: > <resending as plain text> > > On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi<premi@ti.com> wrote: >> >> +#ifdef CONFIG_SMP >> + /* Adjust jiffies before transition */ >> + for_each_cpu(i, policy->cpus) { >> + unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy; >> + >> + per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj, >> + freqs.old, >> + freqs.new); This isn't the right patch since it does suffer from the progressive error. There was an updated patch on this thread from Russell which I re-based. > Can't this rewrite the loops_per_jiffy for the other CPU while it is > in a udelay? If it has already calculated the number of loops > necessary, and the CPU frequency increases, it could end up returning > too early from udelay. > > There were previous discussions about polling a fixed-frequency timer > for udelay on SMP systems. > The udelay code doesn't use the per-cpu lpj variable. It uses the global lpj. Secondly the calibration of no. of loops to be done is precalculateed so overwrite shouldn't impact the scenario you mentioned. Though it has an issue where, pre-calculated loops can become short/long based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. When CPU can scale independently, then we have bigger problem since global lpj based udelay becomes always error prone in all scenario's. So for the OMAP, where the whole CPU cluster is scaled together, I don't see this as a bigger problem. Regards Santosh
On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: > Can't this rewrite the loops_per_jiffy for the other CPU while it is > in a udelay? If it has already calculated the number of loops > necessary, and the CPU frequency increases, it could end up returning > too early from udelay. udelay uses the global loops_per_jiffy.
<resending as plain text again, sorry, mailer troubles> On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > > On 6/28/2011 3:29 PM, Colin Cross wrote: >> >> <resending as plain text> >> >> On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi<premi@ti.com> wrote: >>> > >>> +#ifdef CONFIG_SMP >>> + /* Adjust jiffies before transition */ >>> + for_each_cpu(i, policy->cpus) { >>> + unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy; >>> + >>> + per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj, >>> + freqs.old, >>> + freqs.new); > > This isn't the right patch since it does suffer from the progressive > error. There was an updated patch on this thread from Russell which > I re-based. Both patches have the same issue - they update loops_per_jiffy, which is unsafe on an SMP system. >> >> Can't this rewrite the loops_per_jiffy for the other CPU while it is >> in a udelay? If it has already calculated the number of loops >> necessary, and the CPU frequency increases, it could end up returning >> too early from udelay. >> >> There were previous discussions about polling a fixed-frequency timer >> for udelay on SMP systems. >> > The udelay code doesn't use the per-cpu lpj variable. It uses the global > lpj. Secondly the calibration of no. of loops to be done is > precalculateed so overwrite shouldn't impact the scenario you mentioned. > > Though it has an issue where, pre-calculated loops can become short/long > based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. The precalculated loops is exactly the problem I described. udelay(100) can return in 50 microseconds if the cpu speed is doubled. On OMAP4, frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than 4 times too short. That breaks the guarantees of udelay. > When CPU can scale independently, then we have bigger problem since > global lpj based udelay becomes always error prone in all scenario's. > > So for the OMAP, where the whole CPU cluster is scaled together, > I don't see this as a bigger problem. > > Regards > Santosh > > >
On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: >> Can't this rewrite the loops_per_jiffy for the other CPU while it is >> in a udelay? If it has already calculated the number of loops >> necessary, and the CPU frequency increases, it could end up returning >> too early from udelay. > > udelay uses the global loops_per_jiffy. > The problem is still the same - loops_per_jiffy applies to both CPUs, and the frequency of the other CPU cannot be changed if it is in a udelay.
On 6/28/2011 3:53 PM, Colin Cross wrote: > On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com <mailto:santosh.shilimkar@ti.com>> wrote: [....] > Can't this rewrite the loops_per_jiffy for the other CPU while it is > in a udelay? If it has already calculated the number of loops > necessary, and the CPU frequency increases, it could end up > returning > too early from udelay. > > There were previous discussions about polling a fixed-frequency > timer > for udelay on SMP systems. > > The udelay code doesn't use the per-cpu lpj variable. It uses the global > lpj. Secondly the calibration of no. of loops to be done is > precalculateed so overwrite shouldn't impact the scenario you mentioned. > > Though it has an issue where, pre-calculated loops can become short/long > based on new clock change which impacts both CPU's on OMAP, when the > other CPU is in in the middle of u-delay routine.. > > > The precalculated loops is exactly the problem I described. udelay(100) > can return in 50 microseconds if the cpu speed is doubled. On OMAP4, > frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than > 4 times too short. That breaks the guarantees of udelay. > You have a point and I agree with you on above. And to fix that scenrio, the only option is to use hardware timer based u-delay() which can remain constant across the CPU freq change. Regards, Santosh
On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote: > The udelay code doesn't use the per-cpu lpj variable. It uses the global > lpj. Secondly the calibration of no. of loops to be done is > precalculateed so overwrite shouldn't impact the scenario you mentioned. > > Though it has an issue where, pre-calculated loops can become short/long > based on new clock change which impacts both CPU's on OMAP, when the > other CPU is in in the middle of u-delay routine.. And there's also the issue where you can start a udelay loop on one CPU, be pre-empted and end up running it on a different CPU running at a different speed. The thing to bear in mind is that udelays are approximate at best - I did some investigation into the accuracy of the loops_per_jiffy calculation, and it _will_ produce shorter delays than expected by the fact that what is being calibrated is the udelay() loop _plus_ the timer interrupt overhead.
On 6/28/2011 4:00 PM, Santosh Shilimkar wrote: > On 6/28/2011 3:53 PM, Colin Cross wrote: >> On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar >> <santosh.shilimkar@ti.com <mailto:santosh.shilimkar@ti.com>> wrote: > > [....] > >> Can't this rewrite the loops_per_jiffy for the other CPU while it is >> in a udelay? If it has already calculated the number of loops >> necessary, and the CPU frequency increases, it could end up >> returning >> too early from udelay. >> >> There were previous discussions about polling a fixed-frequency >> timer >> for udelay on SMP systems. >> >> The udelay code doesn't use the per-cpu lpj variable. It uses the global >> lpj. Secondly the calibration of no. of loops to be done is >> precalculateed so overwrite shouldn't impact the scenario you mentioned. >> >> Though it has an issue where, pre-calculated loops can become short/long >> based on new clock change which impacts both CPU's on OMAP, when the >> other CPU is in in the middle of u-delay routine.. >> >> >> The precalculated loops is exactly the problem I described. udelay(100) >> can return in 50 microseconds if the cpu speed is doubled. On OMAP4, >> frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than >> 4 times too short. That breaks the guarantees of udelay. >> > You have a point and I agree with you on above. > And to fix that scenrio, the only option is to use hardware > timer based u-delay() which can remain constant across the > CPU freq change. > Or block CPUFREQ change when any CPU which is in udelay() is done with it, which will be stupid to do :-) Regards Santosh
On 6/28/2011 4:03 PM, Russell King - ARM Linux wrote: > On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote: >> The udelay code doesn't use the per-cpu lpj variable. It uses the global >> lpj. Secondly the calibration of no. of loops to be done is >> precalculateed so overwrite shouldn't impact the scenario you mentioned. >> >> Though it has an issue where, pre-calculated loops can become short/long >> based on new clock change which impacts both CPU's on OMAP, when the >> other CPU is in in the middle of u-delay routine.. > > And there's also the issue where you can start a udelay loop on one CPU, > be pre-empted and end up running it on a different CPU running at a > different speed. > Indeed. > The thing to bear in mind is that udelays are approximate at best - I did > some investigation into the accuracy of the loops_per_jiffy calculation, > and it _will_ produce shorter delays than expected by the fact that > what is being calibrated is the udelay() loop _plus_ the timer interrupt > overhead. > Sure but as Colin pointed out 4 times shorter delay will be too much. Regards Santosh
On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote: > On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: > >> Can't this rewrite the loops_per_jiffy for the other CPU while it is > >> in a udelay? If it has already calculated the number of loops > >> necessary, and the CPU frequency increases, it could end up returning > >> too early from udelay. > > > > udelay uses the global loops_per_jiffy. > > > > The problem is still the same - loops_per_jiffy applies to both CPUs, > and the frequency of the other CPU cannot be changed if it is in a > udelay. If you have a SMP system where both CPUs scale together then you will get both CPUs being impacted, which may result in udelay() terminating well early or taking much longer than was originally intended. That's rather unavoidable with software timing loops - we could add a rw spinlock around udelay, but that would require interrupts to be disabled and that wouldn't be nice in general to have every udelay running with IRQs off. That's why people have proposed hardware-timer based delay loops - these screw up the bogomips value (it no longer refers to the CPU but to the timer used for the delays) and the code proposed thus far probably has a severe negative impact on ARMs running at low clock rates (the calculation cost of the number of loops to run becomes significant for CPUs below 100MHz for short delays with the existing optimized assembler, so moving it into C and introducing function pointers will only make it worse.)
On Tue, Jun 28, 2011 at 4:17 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote: >> On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: >> >> Can't this rewrite the loops_per_jiffy for the other CPU while it is >> >> in a udelay? If it has already calculated the number of loops >> >> necessary, and the CPU frequency increases, it could end up returning >> >> too early from udelay. >> > >> > udelay uses the global loops_per_jiffy. >> > >> >> The problem is still the same - loops_per_jiffy applies to both CPUs, >> and the frequency of the other CPU cannot be changed if it is in a >> udelay. > > If you have a SMP system where both CPUs scale together then you will > get both CPUs being impacted, which may result in udelay() terminating > well early or taking much longer than was originally intended. > > That's rather unavoidable with software timing loops - we could add a > rw spinlock around udelay, but that would require interrupts to be > disabled and that wouldn't be nice in general to have every udelay > running with IRQs off. > > That's why people have proposed hardware-timer based delay loops - > these screw up the bogomips value (it no longer refers to the CPU > but to the timer used for the delays) and the code proposed thus far > probably has a severe negative impact on ARMs running at low clock > rates (the calculation cost of the number of loops to run becomes > significant for CPUs below 100MHz for short delays with the existing > optimized assembler, so moving it into C and introducing function > pointers will only make it worse.) > I don't think it affects bogomips - loops_per_jiffy can still be calibrated and updated by cpufreq, udelay just wont use them. If the pointer dereference is done at the udelay() call to allow each platform to override udelay, slow platforms can continue to use the original optimized assembly with only a few extra instructions overhead on entry.
On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote: > I don't think it affects bogomips - loops_per_jiffy can still be > calibrated and updated by cpufreq, udelay just wont use them. No, you can't avoid it. __delay(), udelay(), and the global loops_per_jiffy are all linked together - for instance this is how the spinlock code has a 1s timeout: static void __spin_lock_debug(raw_spinlock_t *lock) { u64 loops = loops_per_jiffy * HZ; int print_once = 1; for (;;) { for (i = 0; i < loops; i++) { if (arch_spin_trylock(&lock->raw_lock)) return; __delay(1); } which goes wrong for all the same reasons you're pointing out about udelay(). So just restricting your argument to udelay() is not realistic.
On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote: >> I don't think it affects bogomips - loops_per_jiffy can still be >> calibrated and updated by cpufreq, udelay just wont use them. > > No, you can't avoid it. __delay(), udelay(), and the global > loops_per_jiffy are all linked together - for instance this is how > the spinlock code has a 1s timeout: > > static void __spin_lock_debug(raw_spinlock_t *lock) > { > u64 loops = loops_per_jiffy * HZ; > int print_once = 1; > > for (;;) { > for (i = 0; i < loops; i++) { > if (arch_spin_trylock(&lock->raw_lock)) > return; > __delay(1); > } > > which goes wrong for all the same reasons you're pointing out about > udelay(). So just restricting your argument to udelay() is not > realistic. > True, there are a few other users of loops_per_jiffy and __delay, but it looks like __spin_lock_debug is the only one worth worrying about, and it's timing is not as critical as udelay - worst case here is that the warning occurs after 250 ms instead of 1s. Leaving loops_per_jiffy and __delay intact, and fixing udelay, would still be a net gain.
On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote: > On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote: > >> I don't think it affects bogomips - loops_per_jiffy can still be > >> calibrated and updated by cpufreq, udelay just wont use them. > > > > No, you can't avoid it. __delay(), udelay(), and the global > > loops_per_jiffy are all linked together - for instance this is how > > the spinlock code has a 1s timeout: > > > > static void __spin_lock_debug(raw_spinlock_t *lock) > > { > > u64 loops = loops_per_jiffy * HZ; > > int print_once = 1; > > > > for (;;) { > > for (i = 0; i < loops; i++) { > > if (arch_spin_trylock(&lock->raw_lock)) > > return; > > __delay(1); > > } > > > > which goes wrong for all the same reasons you're pointing out about > > udelay(). So just restricting your argument to udelay() is not > > realistic. > > > > True, there are a few other users of loops_per_jiffy and __delay, but > it looks like __spin_lock_debug is the only one worth worrying about, > and it's timing is not as critical as udelay - worst case here is that > the warning occurs after 250 ms instead of 1s. Leaving > loops_per_jiffy and __delay intact, and fixing udelay, would still be > a net gain. Other users of loops_per_jiffy are incorrect in any case: static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, unsigned long bit) { unsigned long i = 0; unsigned long limit = (loops_per_jiffy * msecs_to_jiffies(MMC_TIMEOUT_MS)); ... if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET) { while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit)) && (i++ < limit)) cpu_relax(); } Is rubbish. static void omap_write_buf_pref(struct mtd_info *mtd, const u_char *buf, int len) { ... /* wait for data to flushed-out before reset the prefetch */ tim = 0; limit = (loops_per_jiffy * msecs_to_jiffies(OMAP_NAND_TIMEOUT_MS)); while (gpmc_read_status(GPMC_PREFETCH_COUNT) && (tim++ < limit)) cpu_relax(); Another load of rubbish. static int flush(struct pl022 *pl022) { unsigned long limit = loops_per_jiffy << 1; dev_dbg(&pl022->adev->dev, "flush\n"); do { while (readw(SSP_SR(pl022->virtbase)) & SSP_SR_MASK_RNE) readw(SSP_DR(pl022->virtbase)); } while ((readw(SSP_SR(pl022->virtbase)) & SSP_SR_MASK_BSY) && limit--); Yet more... static int flush(struct driver_data *drv_data) { unsigned long limit = loops_per_jiffy << 1; void __iomem *reg = drv_data->ioaddr; do { while (read_SSSR(reg) & SSSR_RNE) { read_SSDR(reg); } } while ((read_SSSR(reg) & SSSR_BSY) && --limit); and... sound/soc/samsung/i2s.c: #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t) /* Be patient */ val = msecs_to_loops(1) / 1000; /* 1 usec */ while (--val) cpu_relax(); even worse. #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t) static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s) { u32 iiscon; unsigned long loops = msecs_to_loops(5); while (--loops) { iiscon = readl(i2s->regs + S3C2412_IISCON); if (iiscon & S3C2412_IISCON_LRINDEX) break; cpu_relax(); } It just goes on... And strangely the major offender of this stuff are ARM drivers, not other peoples stuff and not stuff in drivers/staging. So I don't think its sane to ignore loops_per_jiffy and __delay, and just concentrate on udelay().
On Wed, Jun 29, 2011 at 7:00 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote: >> On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote: >> >> I don't think it affects bogomips - loops_per_jiffy can still be >> >> calibrated and updated by cpufreq, udelay just wont use them. >> > >> > No, you can't avoid it. __delay(), udelay(), and the global >> > loops_per_jiffy are all linked together - for instance this is how >> > the spinlock code has a 1s timeout: >> > >> > static void __spin_lock_debug(raw_spinlock_t *lock) >> > { >> > u64 loops = loops_per_jiffy * HZ; >> > int print_once = 1; >> > >> > for (;;) { >> > for (i = 0; i < loops; i++) { >> > if (arch_spin_trylock(&lock->raw_lock)) >> > return; >> > __delay(1); >> > } >> > >> > which goes wrong for all the same reasons you're pointing out about >> > udelay(). So just restricting your argument to udelay() is not >> > realistic. >> > >> >> True, there are a few other users of loops_per_jiffy and __delay, but >> it looks like __spin_lock_debug is the only one worth worrying about, >> and it's timing is not as critical as udelay - worst case here is that >> the warning occurs after 250 ms instead of 1s. Leaving >> loops_per_jiffy and __delay intact, and fixing udelay, would still be >> a net gain. > > Other users of loops_per_jiffy are incorrect in any case: The same conclusion I came to on a quick scan of other uses of loops_per_jiffy... <snip> > And strangely the major offender of this stuff are ARM drivers, not other > peoples stuff and not stuff in drivers/staging. > > So I don't think its sane to ignore loops_per_jiffy and __delay, and just > concentrate on udelay(). But this I don't understand. Every other use I found of loops_per_jiffy is incorrect and should be changed. Fixing udelay now would not make them any worse - they would stay just as broken as they were, so there is no need to couple a fix to udelay with fixing other users of loops_per_jiffy. Why block a legitimate fix because some other broken code uses a variable whose behavior would not change? If you are requesting an alternate change that would allow __delay to continue to be used to time loops while irqs are off and jiffies is not being updated, but also allows loops_per_jiffy to change in the middle of a loop, while not increasing the number of instructions executed in __delay, I don't think that is possible. You could replace __delay with a function that tests against a timer, and loops_per_jiffy with the frequency of the counter divided by HZ, but that would limit your spinlock spins to the frequency of the counter - 1MHz on Tegra. Are there ever other legitimate uses of loops_per_jiffy/__delay? I don't think even the spinlock_debug use is correct - the number of instructions executed in the loop before the __delay call (I count 17) is going to be much higher than the instructions in the __delay(1) call (3 instructions). The spinlock debug timeout is already going to be much longer than expected. It looks to me like the only legitimate use of loops_per_jiffy is to calculate the number of loops and call __delay(loops) (exactly what udelay does), the overhead of doing anything else will swamp the __delay call. spinlock debug can get away with its incorrect usage, because it doesn't really care how long the delay is, and must have a minimum overhead.
On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote: > > That's why people have proposed hardware-timer based delay loops - > these screw up the bogomips value (it no longer refers to the CPU > but to the timer used for the delays) and the code proposed thus far > probably has a severe negative impact on ARMs running at low clock > rates (the calculation cost of the number of loops to run becomes > significant for CPUs below 100MHz for short delays with the existing > optimized assembler, so moving it into C and introducing function > pointers will only make it worse.) Am I people? ;-) The code I've proposed doesn't seem to have a negative impact on our targets even when the processor is running at 19.2 Mhz. Before and after the patches I get the same lpj value (this is all described in the commit text). I've also shown that rewriting delay.S into C doesn't negatively affect the hand optimized assembly as the before and after object code is nearly identical modulo register allocation. The only issue would be the one function pointer which I haven't heard anyone complain about until now. Even if the time to get into the __delay() routine increases by a few instructions I don't see how this harms anything as udelay() is a minimum time guarantee. We should strive to make it as close as possible to the time requested by the caller, but we shouldn't balk at the introduction of a few more cycles along the setup path. Finally, since the calibration takes into account most of the new instructions I doubt it will even be noticeable overhead to have the function pointers. What more can I do to convince you to take this patch?
On Wed, Jun 29, 2011 at 11:29:29AM -0700, Stephen Boyd wrote: > On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote: > > > > That's why people have proposed hardware-timer based delay loops - > > these screw up the bogomips value (it no longer refers to the CPU > > but to the timer used for the delays) and the code proposed thus far > > probably has a severe negative impact on ARMs running at low clock > > rates (the calculation cost of the number of loops to run becomes > > significant for CPUs below 100MHz for short delays with the existing > > optimized assembler, so moving it into C and introducing function > > pointers will only make it worse.) > > Am I people? ;-) That depends if you're a multiple personality person! > The code I've proposed doesn't seem to have a negative impact on our > targets even when the processor is running at 19.2 Mhz. Before and after > the patches I get the same lpj value (this is all described in the > commit text). I've also shown that rewriting delay.S into C doesn't > negatively affect the hand optimized assembly as the before and after > object code is nearly identical modulo register allocation. The only > issue would be the one function pointer which I haven't heard anyone > complain about until now. > > Even if the time to get into the __delay() routine increases by a few > instructions I don't see how this harms anything as udelay() is a > minimum time guarantee. We should strive to make it as close as possible > to the time requested by the caller, but we shouldn't balk at the > introduction of a few more cycles along the setup path. Finally, since > the calibration takes into account most of the new instructions I doubt > it will even be noticeable overhead to have the function pointers. > > What more can I do to convince you to take this patch? What I'm aware of is that I did create a kernel-side parport jtag driver, and the limiting factor in that was udelay(), or rather udelay(1) not giving a delay of 1us but several us longer - and that was tracked down to the overhead of the CPU getting into __delay. So, having experienced that problem I'm over-sensitive towards it...
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c index 346519e..0263cae 100644 --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c @@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy, return ret; /* Notify transitions */ - if (is_smp()) { - for_each_cpu(i, policy->cpus) { - freqs.cpu = i; - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); - } - } else { + for_each_cpu(i, policy->cpus) { + freqs.cpu = i; cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); } @@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy, freqs.new = omap_getspeed(policy->cpu); +#ifdef CONFIG_SMP + /* Adjust jiffies before transition */ + for_each_cpu(i, policy->cpus) { + unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy; + + per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj, + freqs.old, + freqs.new); + } +#endif + /* Notify transitions */ - if (is_smp()) { - for_each_cpu(i, policy->cpus) { - freqs.cpu = i; - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); - } - } else { + for_each_cpu(i, policy->cpus) { + freqs.cpu = i; cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); }
Currently, loops_per_jiffy is being calculated twice for non-SMP processors. - Before calling cpufreq_notify_transition() - From within cpufreq_notify_transition() Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in "cat /proc/cpuinfo". The value of loops_per_jiffy needs to be calculated only when CONFIG_SMP is true. It is the core change included in this patch. The patch also leverages the definition of for_each_cpu() with and without CONFIG_SMP to consolidate the mechanism to call cpufreq_notify_transition(). Signed-off-by: Sanjeev Premi <premi@ti.com> --- Changes since v1: * loops_per_jiffy are updated when CONFIG_SMP is true. * leverage definition of for_each_cpu() Tested on OMAP3EVM with and without CONFIG_SMP. Since the log is rather long, will be posting the log in a follow-up mail. arch/arm/mach-omap2/omap2plus-cpufreq.c | 27 +++++++++++++++------------ 1 files changed, 15 insertions(+), 12 deletions(-) -- 1.7.2.2