[PATCH] <down,down_interruptible...>, kernel <3.2.9>

[PATCH] <down,down_interruptible...>, kernel <3.2.9>

[Dennis Chen]

Current down family functions use mismatch spin_lock pairs, this will
incur some interrupt state chaos, for example,
down_interruptible --
     spin_lock_irqsave(&sem->lock, flags);              P1
           __down_common--
                 spin_unlock_irq(&sem->lock);              P2
		timeout = schedule_timeout(timeout);
		spin_lock_irq(&sem->lock);                  P3

     spin_unlock_irqrestore(&sem->lock, flags);       P4

Suppose 2 kernel thread A and B in an UP system call
down_interruptible to get the semaphore, if the irq is _OFF_ before A
calls, in the section between P2 and P3,  the irq will be turned _ON_,
then B begins to call down_interruptible, it will save a flag
indicating irq is _ON_. So after A finish the path of
down_interruptible, the irq is still _OFF_, but when B wakes up and
finish the path, the irq will be _ON_. Actually, irq should be in on
state before any down_interruptible calling, so
spin_lock_irqsave/irqrestore is not necessary. Given it will make
confusion for the reason of unmatched spin_lock pairs between
down_interruptible and __down_common, so it's reason for the patch.
Any comments?

--- semaphore.bak.c     2012-03-02 21:37:50.510302064 +0800
+++ semaphore.c 2012-03-02 21:46:32.330269122 +0800
@@ -54,12 +54,12 @@
 {
        unsigned long flags;

-       raw_spin_lock_irqsave(&sem->lock, flags);
+       raw_spin_lock_irq(&sem->lock, flags);
        if (likely(sem->count > 0))
                sem->count--;
        else
                __down(sem);
-       raw_spin_unlock_irqrestore(&sem->lock, flags);
+       raw_spin_unlock_irq(&sem->lock, flags);
 }
 EXPORT_SYMBOL(down);

@@ -77,12 +77,12 @@
        unsigned long flags;
        int result = 0;

-       raw_spin_lock_irqsave(&sem->lock, flags);
+       raw_spin_lock_irq(&sem->lock, flags);
        if (likely(sem->count > 0))
                sem->count--;
        else
                result = __down_interruptible(sem);
-       raw_spin_unlock_irqrestore(&sem->lock, flags);
+       raw_spin_unlock_irq(&sem->lock, flags);

        return result;
 }
@@ -103,12 +103,12 @@
        unsigned long flags;
        int result = 0;

-       raw_spin_lock_irqsave(&sem->lock, flags);
+       raw_spin_lock_irq(&sem->lock, flags);
        if (likely(sem->count > 0))
                sem->count--;
        else
                result = __down_killable(sem);
-       raw_spin_unlock_irqrestore(&sem->lock, flags);
+       raw_spin_unlock_irq(&sem->lock, flags);

        return result;
 }
@@ -157,12 +157,12 @@
        unsigned long flags;
        int result = 0;

-       raw_spin_lock_irqsave(&sem->lock, flags);
+       raw_spin_lock_irq(&sem->lock, flags);
        if (likely(sem->count > 0))
                sem->count--;
        else
                result = __down_timeout(sem, jiffies);
-       raw_spin_unlock_irqrestore(&sem->lock, flags);
+       raw_spin_unlock_irq(&sem->lock, flags);

        return result;
 }

Re: [PATCH] <down,down_interruptible...>, kernel <3.2.9>

[Arnd Bergmann]

On Friday 02 March 2012, Dennis Chen wrote:
> Current down family functions use mismatch spin_lock pairs, this will
> incur some interrupt state chaos, for example,
> down_interruptible --
>      spin_lock_irqsave(&sem->lock, flags);              P1
>            __down_common--
>                  spin_unlock_irq(&sem->lock);              P2
>                 timeout = schedule_timeout(timeout);
>                 spin_lock_irq(&sem->lock);                  P3
> 
>      spin_unlock_irqrestore(&sem->lock, flags);       P4
> 
> Suppose 2 kernel thread A and B in an UP system call
> down_interruptible to get the semaphore, if the irq is OFF before A
> calls, in the section between P2 and P3,  the irq will be turned _ON_,
> then B begins to call down_interruptible, it will save a flag
> indicating irq is _ON_. So after A finish the path of
> down_interruptible, the irq is still _OFF_, but when B wakes up and
> finish the path, the irq will be _ON_. Actually, irq should be in on
> state before any down_interruptible calling, so
> spin_lock_irqsave/irqrestore is not necessary. Given it will make
> confusion for the reason of unmatched spin_lock pairs between
> down_interruptible and __down_common, so it's reason for the patch.
> Any comments?

You are right that the spin_lock_irqsave is unnecessary because
down() can only be called in non-atomic state with interrupts enabled.
Your patch will be tiny performance improvement on architectures where
saving the interrupt state does not come for free.

However, there is nothing in the code where we can end up with an
unexpected state, because both threads have their own copy of the
'flags' variable, which always contains the enabled state that gets
restored upon leaving the functions.

	Arnd

Re: [PATCH] <down,down_interruptible...>, kernel <3.2.9>

[Dennis Chen]

On Fri, Mar 2, 2012 at 11:22 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 02 March 2012, Dennis Chen wrote:
>> Current down family functions use mismatch spin_lock pairs, this will
>> incur some interrupt state chaos, for example,
>> down_interruptible --
>>      spin_lock_irqsave(&sem->lock, flags);              P1
>>            __down_common--
>>                  spin_unlock_irq(&sem->lock);              P2
>>                 timeout = schedule_timeout(timeout);
>>                 spin_lock_irq(&sem->lock);                  P3
>>
>>      spin_unlock_irqrestore(&sem->lock, flags);       P4
>>
>> Suppose 2 kernel thread A and B in an UP system call
>> down_interruptible to get the semaphore, if the irq is OFF before A
>> calls, in the section between P2 and P3,  the irq will be turned _ON_,
>> then B begins to call down_interruptible, it will save a flag
>> indicating irq is _ON_. So after A finish the path of
>> down_interruptible, the irq is still _OFF_, but when B wakes up and
>> finish the path, the irq will be _ON_. Actually, irq should be in on
>> state before any down_interruptible calling, so
>> spin_lock_irqsave/irqrestore is not necessary. Given it will make
>> confusion for the reason of unmatched spin_lock pairs between
>> down_interruptible and __down_common, so it's reason for the patch.
>> Any comments?
>
> You are right that the spin_lock_irqsave is unnecessary because
> down() can only be called in non-atomic state with interrupts enabled.
> Your patch will be tiny performance improvement on architectures where
> saving the interrupt state does not come for free.
>
> However, there is nothing in the code where we can end up with an
> unexpected state, because both threads have their own copy of the
> 'flags' variable, which always contains the enabled state that gets
> restored upon leaving the functions.
>
>        Arnd

Arnd, thanks for the clarification. It's the truth that every thread
has its own copy of 'flags', so if the down() can only be called with
interrupts enabled, that's ok for the code as the way.

        Dennis