On Wed, Jun 29, 2011 at 7:00 AM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
