* Nicholas Mc Guire | 2013-11-22 05:44:39 [+0100]:
> rt_write_trylock_irqsave unconditionally calls rt_write_trylock which will
> disable migration if the lock was sucessfully acquired.
> This patch drops the recursive migrate_disable/enable in
> rt_write_trylock_irqsave and rt_write_unlock_irq respecttively
>
> No change of functionality
I think you change functionality because you remove code. If you would
cut a big function into two small functions then I would agree that the
functionality has not been changed. But…
>index 9a5fe26..f6c7612 100644
>--- a/include/linux/rwlock_rt.h
>+++ b/include/linux/rwlock_rt.h
>@@ -105,7 +105,6 @@ extern void __rt_rwlock_init(rwlock_t *rwlock, char *name, struct lock_class_key
> typecheck(unsigned long, flags); \
> (void) flags; \
> rt_read_unlock(lock); \
>- migrate_enable(); \
> } while (0)
You are removing that from read_unlock_irqrestore() instead of
write_unlock_irqrestore() which comes next:
>
> #define write_unlock_irqrestore(lock, flags) \
>diff --git a/kernel/rt.c b/kernel/rt.c
>index 4b2c4a9..71d26a4 100644
>--- a/kernel/rt.c
>+++ b/kernel/rt.c
>@@ -196,10 +196,8 @@ int __lockfunc rt_write_trylock_irqsave(rwlock_t *rwlock, unsigned long *flags)
> int ret;
>
> *flags = 0;
>- migrate_disable();
> ret = rt_write_trylock(rwlock);
>- if (!ret)
>- migrate_enable();
>+
The amount of users of rt_write_trylock_irqsave() is so small that
nobody noticed this bug:
write_trylock_irqsave()
->rt_write_trylock_irqsave()
-> m_d()
-> rt_write_trylock()
-> m_d()
That means we called migrate_disable() twice. Now comes the unlock path:
write_unlock_irqrestore()
-> rt_write_unlock()
-> m_e()
That means migrations still needs to be enabled.
I would however prefer your optimisation where you remove the pointless
m_e() in rt_write_trylock_irqsave();
> return ret;
> }
> EXPORT_SYMBOL(rt_write_trylock_irqsave);
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
