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

[Chen Gang <gang.chen@asianux.com>]

On 06/19/2013 04:41 PM, Thomas Gleixner wrote:
> On Wed, 19 Jun 2013, Chen Gang wrote:
> 
>> > 
>> > When CONFIG_LOCKDEP is not defined, spin_lock_irqsave() is not equal to
>> > spin_lock() + local_irq_save().
>> > 
>> > In __mod_timer(), After call spin_lock_irqsave() with 'base->lock' in
>> > lock_timer_base(), it may use spin_lock() with the 'new_base->lock'.
>> > 
>> > It may let original call do_raw_spin_lock_flags() with 'base->lock',
>> > but new  call LOCK_CONTENDED() with 'new_base->lock'.
>> > 
>> > In fact, we need both of them call do_raw_spin_lock_flags(), so use
>> > spin_lock_irqsave() instead of spin_lock() + local_irq_save().
> Why do we need to do that? There is no reason to do so and it's
> totally irrelevant whether CONFIG_LOCKDEP is enabled or not.
>

Please see include/linux/spinlock_api_smp.h (or see bottom of this mail)

 
> The code is written intentionally this way.
> 
> What's the difference between:
> 
>        spin_lock_irqsave(&l1, flags);
>        spin_unlock(&l1);
>        spin_lock(l2);
>        spin_unlock_irqrestore(&l2, flags);
> 
> and
> 
>        spin_lock_irqsave(&l1, flags);
>        spin_unlock_irqrestore(&l1);
>        spin_lock_irqsave(l2, flags);
>        spin_unlock_irqrestore(&l2, flags);
> 

Yes


> The difference is that we avoid to touch the interrupt disable in the
> cpu, which might be an expensive operation depending on the cpu model.
> 
> There is no point in reenabling interrupts just to disable them
> again a few instruction cycles later.
> 
> And lockdep is perfectly fine with that code. All lockdep cares about
> is whether the lock context (interrupts disabled) is correct or
> not.

Please reference include/linux/spinlock_api_smp.h and include/linux/spinlock.h

  spin_lock_irqsave() ->
    raw_spin_lock_irqsave() ->
      _raw_spin_lock_irqsave() ->
        __raw_spin_lock_irqsave()

  spin_lock() ->
    raw_spin_lock() ->
      _raw_spin_lock() ->
        __raw_spin_lock()


104 static inline unsigned long __raw_spin_lock_irqsave(raw_spinlock_t *lock)
105 {
106         unsigned long flags;
107 
108         local_irq_save(flags);
109         preempt_disable();
110         spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
111         /*
112          * On lockdep we dont want the hand-coded irq-enable of
113          * do_raw_spin_lock_flags() code, because lockdep assumes
114          * that interrupts are not re-enabled during lock-acquire:
115          */
116 #ifdef CONFIG_LOCKDEP
117         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
118 #else
119         do_raw_spin_lock_flags(lock, &flags);
120 #endif
121         return flags;
122 }
...

140 static inline void __raw_spin_lock(raw_spinlock_t *lock)
141 {
142         preempt_disable();
143         spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
144         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
145 }



Thanks.
-- 
Chen Gang

Asianux Corporation