Messages in this thread | | | Date | Wed, 19 Jun 2013 12:49:10 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined |
| |
On Wed, 19 Jun 2013, Chen Gang wrote: > On 06/19/2013 05:59 PM, Thomas Gleixner wrote: > > Lockdep tracks lock ordering and the context in which a lock is > > taken. The timer base lock can be taken in interrupt context, so it > > always needs to be taken with interrupts disabled. That's what lockdep > > cares about. > > > > And > > spin_lock_irqsave(&l1, flags); > > spin_unlock(&l1); > > spin_lock(&l2); > > spin_unlock_irqrestore(&l2, flags); > > > > fulfils that for both l1 and l2. > > > > It does not matter whether the code pathes are different, what matters > > is that they are semantically the same. And that's the case. > > But I feel, they are not semantically the same.
This is not about feelings. This is about facts.
> if CONFIG_LOCKDEP is not defined, spin_lock_irqsave() should call > do_raw_spin_lock_flags(), not call LOCK_CONTENDED().
That is what the code already does:
#ifdef CONFIG_LOCKDEP LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock); #else do_raw_spin_lock_flags(lock, &flags); #endif
If LOCKDEP is not defined it calls do_raw_spin_lock_flags(). Where is your point?
And the reason why the LOCKDEP case and the !LOCKDEP case are different is explained in the comment above that code. And both behave the same way versus the spinlock semantics.
We must do this because some architectures implement do_raw_spin_lock_flags() in the following way:
do_raw_spin_lock_flags(l, flags) { while (!arch_spin_trylock(l)) { if (!irq_disabled_flags(flags)) { arch_irq_restore(flags); cpu_relax(); arch_irq_disable(); } } }
This is done to limit irq disabled time while we wait for the lock. Though most architectures implement it as:
while (!arch_spin_trylock(l)) cpu_relax();
In the lockdep case we CANNOT reenable interrupts in the spinning code and therefor we use the
while (!arch_spin_trylock(l)) cpu_relax();
variant.
And again. Both are semantically the same.
spin_lock_irqsave() semantics are:
The function returns with the lock acquired, interrupts and preemption disabled. Both variants do that.
The internal details whether an architecture reenables interrupts while spinning on a contended lock or not are completely irrelevant and do not affect the correctness of the code.
Thanks,
tglx
| |