Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner <tglx@linutronix.de>]

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