Re: [PATCH] drop recursive migrate_disable in rt_write_trylock_irqsave

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

* 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html