[PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline

[PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline

[Steven Rostedt]

Looking at mainline's down_read() I noticed that reader locks of rwsems
are not made to nest. In fact, they should not. Although, it may seem
fine if a down_read() nests as multiple readers can have the lock,
rwsems are fair locks. That is, if a writer were to block on a rwsem
while readers have the lock, a new reader will also block. If a reader
were to try to take the lock again while a writer was waiting, it
would block, and cause a deadlock as it has the lock its trying
to grab and wont let it go as the writer is waiting.

I also found that the rt_mutex_init() is identical in the two places
it is defined in rtmutex.h.

Steven Rostedt (Red Hat) (2):
      rwsem-rt: Do not allow readers to nest
      rtmutex: Remove duplicate rt_mutex_init()

----
 include/linux/rtmutex.h  | 12 +++---------
 include/linux/rwsem_rt.h |  1 -
 kernel/rt.c              | 37 ++++++++-----------------------------
 3 files changed, 11 insertions(+), 39 deletions(-)

[PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Steven Rostedt]

[-- Attachment #1: 0001-rwsem-rt-Do-not-allow-readers-to-nest.patch --]
[-- Type: text/plain, Size: 2834 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The readers of mainline rwsems are not allowed to nest, the rwsems in the
PREEMPT_RT kernel should not nest either.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/rwsem_rt.h |  1 -
 kernel/rt.c              | 37 ++++++++-----------------------------
 2 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/include/linux/rwsem_rt.h b/include/linux/rwsem_rt.h
index e94d945..a81151c 100644
--- a/include/linux/rwsem_rt.h
+++ b/include/linux/rwsem_rt.h
@@ -20,7 +20,6 @@
 
 struct rw_semaphore {
 	struct rt_mutex		lock;
-	int			read_depth;
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map	dep_map;
 #endif
diff --git a/kernel/rt.c b/kernel/rt.c
index 5d17727..bb72347 100644
--- a/kernel/rt.c
+++ b/kernel/rt.c
@@ -316,10 +316,8 @@ EXPORT_SYMBOL(rt_up_write);
 
 void  rt_up_read(struct rw_semaphore *rwsem)
 {
-	if (--rwsem->read_depth == 0) {
-		rwsem_release(&rwsem->dep_map, 1, _RET_IP_);
-		rt_mutex_unlock(&rwsem->lock);
-	}
+	rwsem_release(&rwsem->dep_map, 1, _RET_IP_);
+	rt_mutex_unlock(&rwsem->lock);
 }
 EXPORT_SYMBOL(rt_up_read);
 
@@ -330,7 +328,6 @@ EXPORT_SYMBOL(rt_up_read);
 void  rt_downgrade_write(struct rw_semaphore *rwsem)
 {
 	BUG_ON(rt_mutex_owner(&rwsem->lock) != current);
-	rwsem->read_depth = 1;
 }
 EXPORT_SYMBOL(rt_downgrade_write);
 
@@ -367,37 +364,20 @@ void rt_down_write_nested_lock(struct rw_semaphore *rwsem,
 
 int  rt_down_read_trylock(struct rw_semaphore *rwsem)
 {
-	struct rt_mutex *lock = &rwsem->lock;
-	int ret = 1;
-
-	/*
-	 * recursive read locks succeed when current owns the rwsem,
-	 * but not when read_depth == 0 which means that the rwsem is
-	 * write locked.
-	 */
-	if (rt_mutex_owner(lock) != current) {
-		ret = rt_mutex_trylock(&rwsem->lock);
-		if (ret)
-			rwsem_acquire(&rwsem->dep_map, 0, 1, _RET_IP_);
-	} else if (!rwsem->read_depth) {
-		ret = 0;
-	}
+	int ret;
 
+	ret = rt_mutex_trylock(&rwsem->lock);
 	if (ret)
-		rwsem->read_depth++;
+		rwsem_acquire(&rwsem->dep_map, 0, 1, _RET_IP_);
+
 	return ret;
 }
 EXPORT_SYMBOL(rt_down_read_trylock);
 
 static void __rt_down_read(struct rw_semaphore *rwsem, int subclass)
 {
-	struct rt_mutex *lock = &rwsem->lock;
-
-	if (rt_mutex_owner(lock) != current) {
-		rwsem_acquire(&rwsem->dep_map, subclass, 0, _RET_IP_);
-		rt_mutex_lock(&rwsem->lock);
-	}
-	rwsem->read_depth++;
+	rwsem_acquire(&rwsem->dep_map, subclass, 0, _RET_IP_);
+	rt_mutex_lock(&rwsem->lock);
 }
 
 void  rt_down_read(struct rw_semaphore *rwsem)
@@ -422,7 +402,6 @@ void  __rt_rwsem_init(struct rw_semaphore *rwsem, const char *name,
 	debug_check_no_locks_freed((void *)rwsem, sizeof(*rwsem));
 	lockdep_init_map(&rwsem->dep_map, name, key, 0);
 #endif
-	rwsem->read_depth = 0;
 	rwsem->lock.save_state = 0;
 }
 EXPORT_SYMBOL(__rt_rwsem_init);
-- 
1.8.5.3

[PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Steven Rostedt]

[-- Attachment #1: 0001-rwsem-rt-Do-not-allow-readers-to-nest.patch --]
[-- Type: text/plain, Size: 2220 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The readers of mainline rwsems are not allowed to nest, the rwsems in the
PREEMPT_RT kernel should not nest either.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/rwsem_rt.h |  1 -
 kernel/rt.c              | 37 ++++++++-----------------------------
 2 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/include/linux/rwsem_rt.h b/include/linux/rwsem_rt.h
index e94d945..a81151c 100644
--- a/include/linux/rwsem_rt.h
+++ b/include/linux/rwsem_rt.h
@@ -20,7 +20,6 @@
 
 struct rw_semaphore {
 	struct rt_mutex		lock;
-	int			read_depth;
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map	dep_map;
 #endif
diff --git a/kernel/rt.c b/kernel/rt.c
index 5d17727..bb72347 100644
--- a/kernel/rt.c
+++ b/kernel/rt.c
@@ -316,10 +316,8 @@ EXPORT_SYMBOL(rt_up_write);
 
 void  rt_up_read(struct rw_semaphore *rwsem)
 {
-	if (--rwsem->read_depth == 0) {
-		rwsem_release(&rwsem->dep_map, 1, _RET_IP_);
-		rt_mutex_unlock(&rwsem->lock);
-	}
+	rwsem_release(&rwsem->dep_map, 1, _RET_IP_);
+	rt_mutex_unlock(&rwsem->lock);
 }
 EXPORT_SYMBOL(rt_up_read);
 
@@ -330,7 +328,6 @@ EXPORT_SYMBOL(rt_up_read);
 void  rt_downgrade_write(struct rw_semaphore *rwsem)
 {
 	BUG_ON(rt_mutex_owner(&rwsem->lock) != current);
-	rwsem->read_depth = 1;
 }
 EXPORT_SYMBOL(rt_downgrade_write);
 
@@ -367,37 +364,20 @@ void rt_down_write_nested_lock(struct rw_semaphore *rwsem,
 
 int  rt_down_read_trylock(struct rw_semaphore *rwsem)
 {
-	struct rt_mutex *lock = &rwsem->lock;
-	int ret = 1;
-
-	/*
-	 * recursive read locks succeed when current owns the rwsem,
-	 * but not when read_depth == 0 which means that the rwsem is
-	 * write locked.
-	 */
-	if (rt_mutex_owner(lock) != current) {
-		ret = rt_mutex_trylock(&rwsem->lock);
-		if (ret)
-			rwsem_acquire(&rwsem->dep_map, 0, 1, _RET_IP_);
-	} else if (!rwsem->read_depth) {
-		ret = 0;
-	}
+	int ret;
 
+	ret = rt_mutex_trylock(&rwsem->lock);
 	if (ret)
-		rwsem->read_depth++;
+		rwsem_acquire(&rwsem->dep_map, 0, 1, _RET_IP_);
+
 	return ret;
 }
 EXPORT_SYMBOL(rt_down_read_trylock);
 
 static void __rt_down_read(struct rw_semaphore *rwsem, int subclass)
 {
-	struct rt_mutex *lock = &rwsem->lock;

[PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()

[Steven Rostedt]

[-- Attachment #1: 0002-rtmutex-Remove-duplicate-rt_mutex_init.patch --]
[-- Type: text/plain, Size: 1369 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The rt_mutex_init() macro is the same whether or not
CONFIG_DEBUG_RT_MUTEX is set. Remove the duplicate.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/rtmutex.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
index fa18682..f7e79e8 100644
--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -62,25 +62,19 @@ struct hrtimer_sleeper;
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
 
-# define rt_mutex_init(mutex)					\
-	do {							\
-		raw_spin_lock_init(&(mutex)->wait_lock);	\
-		__rt_mutex_init(mutex, #mutex);			\
-	} while (0)
-
  extern void rt_mutex_debug_task_free(struct task_struct *tsk);
 #else
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname)
 
+# define rt_mutex_debug_task_free(t)			do { } while (0)
+#endif
+
 # define rt_mutex_init(mutex)					\
 	do {							\
 		raw_spin_lock_init(&(mutex)->wait_lock);	\
 		__rt_mutex_init(mutex, #mutex);			\
 	} while (0)
 
-# define rt_mutex_debug_task_free(t)			do { } while (0)
-#endif
-
 #define __RT_MUTEX_INITIALIZER_PLAIN(mutexname) \
 	.wait_lock = __RAW_SPIN_LOCK_UNLOCKED(mutexname.wait_lock) \
 	, .wait_list = PLIST_HEAD_INIT(mutexname.wait_list) \
-- 
1.8.5.3

[PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()

[Steven Rostedt]

[-- Attachment #1: 0002-rtmutex-Remove-duplicate-rt_mutex_init.patch --]
[-- Type: text/plain, Size: 1183 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The rt_mutex_init() macro is the same whether or not
CONFIG_DEBUG_RT_MUTEX is set. Remove the duplicate.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/rtmutex.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
index fa18682..f7e79e8 100644
--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -62,25 +62,19 @@ struct hrtimer_sleeper;
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
 
-# define rt_mutex_init(mutex)					\
-	do {							\
-		raw_spin_lock_init(&(mutex)->wait_lock);	\
-		__rt_mutex_init(mutex, #mutex);			\
-	} while (0)
-
  extern void rt_mutex_debug_task_free(struct task_struct *tsk);
 #else
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname)
 
+# define rt_mutex_debug_task_free(t)			do { } while (0)
+#endif
+
 # define rt_mutex_init(mutex)					\
 	do {							\
 		raw_spin_lock_init(&(mutex)->wait_lock);	\
 		__rt_mutex_init(mutex, #mutex);			\
 	} while (0)
 
-# define rt_mutex_debug_task_free(t)			do { } while (0)
-#endif

Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Sebastian Andrzej Siewior]

* Steven Rostedt | 2014-04-08 22:47:01 [-0400]:

>From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
>The readers of mainline rwsems are not allowed to nest, the rwsems in the
>PREEMPT_RT kernel should not nest either.
>
>Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Applied

Sebastian

Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()

[Sebastian Andrzej Siewior]

* Steven Rostedt | 2014-04-08 22:47:02 [-0400]:

>From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
>The rt_mutex_init() macro is the same whether or not
>CONFIG_DEBUG_RT_MUTEX is set. Remove the duplicate.
>
>Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

This does not apply cleanly on v3.14-rt

>--- a/include/linux/rtmutex.h
>+++ b/include/linux/rtmutex.h
>@@ -62,25 +62,19 @@ struct hrtimer_sleeper;
> # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
> 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
> 
>-# define rt_mutex_init(mutex)					\
>-	do {							\
>-		raw_spin_lock_init(&(mutex)->wait_lock);	\
>-		__rt_mutex_init(mutex, #mutex);			\
>-	} while (0)
>-

The macro is the same in CONFIG_DEBUG_RT_MUTEXES and the else path.
Shouldn't you remove both and define it before the ifdef? There are some
users of this:
|drivers/i2c/i2c-core.c: rt_mutex_init(&adap->bus_lock);
|drivers/media/usb/em28xx/em28xx-cards.c: rt_mutex_init(&dev->i2c_bus_lock);

Sebastian

Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()

[Steven Rostedt]

On Fri, 2 May 2014 11:08:31 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> * Steven Rostedt | 2014-04-08 22:47:02 [-0400]:
> 
> >From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >
> >The rt_mutex_init() macro is the same whether or not
> >CONFIG_DEBUG_RT_MUTEX is set. Remove the duplicate.
> >
> >Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> This does not apply cleanly on v3.14-rt

That's because I sent this out before 3.14-rt was released :-)

> 
> >--- a/include/linux/rtmutex.h
> >+++ b/include/linux/rtmutex.h
> >@@ -62,25 +62,19 @@ struct hrtimer_sleeper;
> > # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
> > 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
> > 
> >-# define rt_mutex_init(mutex)					\
> >-	do {							\
> >-		raw_spin_lock_init(&(mutex)->wait_lock);	\
> >-		__rt_mutex_init(mutex, #mutex);			\
> >-	} while (0)
> >-
> 
> The macro is the same in CONFIG_DEBUG_RT_MUTEXES and the else path.
> Shouldn't you remove both and define it before the ifdef?

That's exactly what the patch did! Except that it defined it *after* the ifdef.

Here's the patch again (as it was cut in the reply):

--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -62,25 +62,19 @@ struct hrtimer_sleeper;
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
 
-# define rt_mutex_init(mutex)					\
-	do {							\
-		raw_spin_lock_init(&(mutex)->wait_lock);	\
-		__rt_mutex_init(mutex, #mutex);			\
-	} while (0)
-
  extern void rt_mutex_debug_task_free(struct task_struct *tsk);
 #else
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname)
 
+# define rt_mutex_debug_task_free(t)			do { } while (0)
+#endif
+
 # define rt_mutex_init(mutex)					\
 	do {							\
 		raw_spin_lock_init(&(mutex)->wait_lock);	\
 		__rt_mutex_init(mutex, #mutex);			\
 	} while (0)
 
-# define rt_mutex_debug_task_free(t)			do { } while (0)
-#endif
-
 #define __RT_MUTEX_INITIALIZER_PLAIN(mutexname) \
 	.wait_lock = __RAW_SPIN_LOCK_UNLOCKED(mutexname.wait_lock) \
 	, .wait_list = PLIST_HEAD_INIT(mutexname.wait_list) \

Take special note of the movement of the "#endif".

-- Steve



> There are some
> users of this:
> |drivers/i2c/i2c-core.c: rt_mutex_init(&adap->bus_lock);
> |drivers/media/usb/em28xx/em28xx-cards.c: rt_mutex_init(&dev->i2c_bus_lock);
> 
> Sebastian

Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()

[Sebastian Andrzej Siewior]

On 05/02/2014 03:12 PM, Steven Rostedt wrote:
>> This does not apply cleanly on v3.14-rt
> 
> That's because I sent this out before 3.14-rt was released :-)

Ach right. Sorry, my memory…

>>> --- a/include/linux/rtmutex.h
>>> +++ b/include/linux/rtmutex.h
>>> @@ -62,25 +62,19 @@ struct hrtimer_sleeper;
>>> # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
>>> 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
>>>
>>> -# define rt_mutex_init(mutex)					\
>>> -	do {							\
>>> -		raw_spin_lock_init(&(mutex)->wait_lock);	\
>>> -		__rt_mutex_init(mutex, #mutex);			\
>>> -	} while (0)
>>> -
>>
>> The macro is the same in CONFIG_DEBUG_RT_MUTEXES and the else path.
>> Shouldn't you remove both and define it before the ifdef?
> 
> That's exactly what the patch did! Except that it defined it *after* the ifdef.
> 
> Here's the patch again (as it was cut in the reply):
> 
> --- a/include/linux/rtmutex.h
> +++ b/include/linux/rtmutex.h
> @@ -62,25 +62,19 @@ struct hrtimer_sleeper;
>  # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
>  	, .name = #mutexname, .file = __FILE__, .line = __LINE__
>  
> -# define rt_mutex_init(mutex)					\
> -	do {							\
> -		raw_spin_lock_init(&(mutex)->wait_lock);	\
> -		__rt_mutex_init(mutex, #mutex);			\
> -	} while (0)
> -
>   extern void rt_mutex_debug_task_free(struct task_struct *tsk);
>  #else
>  # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname)
>  
> +# define rt_mutex_debug_task_free(t)			do { } while (0)
> +#endif
> +
>  # define rt_mutex_init(mutex)					\
>  	do {							\
>  		raw_spin_lock_init(&(mutex)->wait_lock);	\
>  		__rt_mutex_init(mutex, #mutex);			\
>  	} while (0)
>  
> -# define rt_mutex_debug_task_free(t)			do { } while (0)
> -#endif
> -
>  #define __RT_MUTEX_INITIALIZER_PLAIN(mutexname) \
>  	.wait_lock = __RAW_SPIN_LOCK_UNLOCKED(mutexname.wait_lock) \
>  	, .wait_list = PLIST_HEAD_INIT(mutexname.wait_list) \
> 
> Take special note of the movement of the "#endif".

Ah. That is the trick. So lets try it again…

> -- Steve

Sebastian

Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Sebastian Andrzej Siewior]

* Steven Rostedt | 2014-04-08 22:47:01 [-0400]:

>From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
>The readers of mainline rwsems are not allowed to nest, the rwsems in the
>PREEMPT_RT kernel should not nest either.

I applied this and this is the reason why cpufreq isn't working. What I
see in cpufreq is:
|         test.sh-788   [004] .......    61.416288: store: down_read_try
|         test.sh-788   [004] .......    61.416296: cpufreq_cpu_get: down_read_try
|         test.sh-788   [004] .......    61.416301: cpufreq_cpu_put.part.6: up_read
|         test.sh-788   [004] .......    61.416332: store: up_read

as you see, one code path takes the read path of rw_sema twice.

Looking at the generic implementation, we have:
|#define RWSEM_UNLOCKED_VALUE            0x00000000L
|#define RWSEM_ACTIVE_BIAS               0x00000001L
|#define RWSEM_WAITING_BIAS              (-RWSEM_ACTIVE_MASK-1)

| static inline int __down_read_trylock(struct rw_semaphore *sem)
| {
|         long tmp;
| 
|         while ((tmp = sem->count) >= 0) {
|                 if (tmp == cmpxchg(&sem->count, tmp,
|                                    tmp + RWSEM_ACTIVE_READ_BIAS)) {
|                         return 1;
|                 }
|         }
|         return 0;
| }

While sem->count is >= 0 we loop and take the semaphore. So we can have
five readers at once. The first writer would set count to a negative
value resulting in trylock failure.

|static inline void __down_read(struct rw_semaphore *sem)
|{
|        if (unlikely(atomic_long_inc_return((atomic_long_t*)&sem->count) <= 0))
|                rwsem_down_read_failed(sem);
|}

Here the same thing but without cmpxchg(). _If_ after an increment the
value is negative then we take slowpath. Otherwise we have the lock.

I think I'm going to revert this patch. Where is it written that
multiple readers of a RW-semaphore can not nest? According to the code
we can even have multiple readers without nesting (two+ processes may
take a reader lock).

Sebastian

Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Steven Rostedt]

On Wed, 18 Feb 2015 20:57:10 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> * Steven Rostedt | 2014-04-08 22:47:01 [-0400]:
> 
> >From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >
> >The readers of mainline rwsems are not allowed to nest, the rwsems in the
> >PREEMPT_RT kernel should not nest either.
> 
> I applied this and this is the reason why cpufreq isn't working. What I
> see in cpufreq is:
> |         test.sh-788   [004] .......    61.416288: store: down_read_try
> |         test.sh-788   [004] .......    61.416296: cpufreq_cpu_get: down_read_try
> |         test.sh-788   [004] .......    61.416301: cpufreq_cpu_put.part.6: up_read
> |         test.sh-788   [004] .......    61.416332: store: up_read
> 
> as you see, one code path takes the read path of rw_sema twice.
> 
> Looking at the generic implementation, we have:
> |#define RWSEM_UNLOCKED_VALUE            0x00000000L
> |#define RWSEM_ACTIVE_BIAS               0x00000001L
> |#define RWSEM_WAITING_BIAS              (-RWSEM_ACTIVE_MASK-1)
> 
> | static inline int __down_read_trylock(struct rw_semaphore *sem)
> | {
> |         long tmp;
> | 
> |         while ((tmp = sem->count) >= 0) {
> |                 if (tmp == cmpxchg(&sem->count, tmp,
> |                                    tmp + RWSEM_ACTIVE_READ_BIAS)) {
> |                         return 1;
> |                 }
> |         }
> |         return 0;
> | }
> 
> While sem->count is >= 0 we loop and take the semaphore. So we can have
> five readers at once. The first writer would set count to a negative
> value resulting in trylock failure.
> 
> |static inline void __down_read(struct rw_semaphore *sem)
> |{
> |        if (unlikely(atomic_long_inc_return((atomic_long_t*)&sem->count) <= 0))
> |                rwsem_down_read_failed(sem);
> |}
> 
> Here the same thing but without cmpxchg(). _If_ after an increment the
> value is negative then we take slowpath. Otherwise we have the lock.

OK, so I need to make it so it can nest with trylock. I have to look at
the patch again because it has been a while.

> 
> I think I'm going to revert this patch. Where is it written that
> multiple readers of a RW-semaphore can not nest? According to the code
> we can even have multiple readers without nesting (two+ processes may
> take a reader lock).

An RW sem must not do two down_read()s on the same lock (it's fine for
a trylock if it has a fail safe for it). The reason is, the second
down_read() will block if there's a writer waiting. Thus you are
guaranteed a deadlock if you have the lock for read, a write comes in
and waits, and you grab the RW sem again, because it will block, and
the writer is waiting for the reader to release. Thus you have a
deadlock.

I'll have to revisit this. I also need to revisit the multi readers
(although Thomas hates it, but he even admitted there's a better way to
do it. Now only if I could remember what that was ;-)

Thanks,

-- Steve

Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Jason Low]

On Wed, Feb 18, 2015 at 12:13 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 18 Feb 2015 20:57:10 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
>> * Steven Rostedt | 2014-04-08 22:47:01 [-0400]:
>>
>> >From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>> >
>> >The readers of mainline rwsems are not allowed to nest, the rwsems in the
>> >PREEMPT_RT kernel should not nest either.
>>
>> I applied this and this is the reason why cpufreq isn't working. What I
>> see in cpufreq is:
>> |         test.sh-788   [004] .......    61.416288: store: down_read_try
>> |         test.sh-788   [004] .......    61.416296: cpufreq_cpu_get: down_read_try
>> |         test.sh-788   [004] .......    61.416301: cpufreq_cpu_put.part.6: up_read
>> |         test.sh-788   [004] .......    61.416332: store: up_read
>>
>> as you see, one code path takes the read path of rw_sema twice.
>>
>> Looking at the generic implementation, we have:
>> |#define RWSEM_UNLOCKED_VALUE            0x00000000L
>> |#define RWSEM_ACTIVE_BIAS               0x00000001L
>> |#define RWSEM_WAITING_BIAS              (-RWSEM_ACTIVE_MASK-1)
>>
>> | static inline int __down_read_trylock(struct rw_semaphore *sem)
>> | {
>> |         long tmp;
>> |
>> |         while ((tmp = sem->count) >= 0) {
>> |                 if (tmp == cmpxchg(&sem->count, tmp,
>> |                                    tmp + RWSEM_ACTIVE_READ_BIAS)) {
>> |                         return 1;
>> |                 }
>> |         }
>> |         return 0;
>> | }
>>
>> While sem->count is >= 0 we loop and take the semaphore. So we can have
>> five readers at once. The first writer would set count to a negative
>> value resulting in trylock failure.
>>
>> |static inline void __down_read(struct rw_semaphore *sem)
>> |{
>> |        if (unlikely(atomic_long_inc_return((atomic_long_t*)&sem->count) <= 0))
>> |                rwsem_down_read_failed(sem);
>> |}
>>
>> Here the same thing but without cmpxchg(). _If_ after an increment the
>> value is negative then we take slowpath. Otherwise we have the lock.
>
> OK, so I need to make it so it can nest with trylock. I have to look at
> the patch again because it has been a while.

When we reported this a few months ago, Thomas provided the following
patch to fix the issue (which essentially reverted the patch) and
appeared to be agreed on:

https://lkml.org/lkml/2014/11/5/844

Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest

[Sebastian Andrzej Siewior]

On 02/18/2015 09:13 PM, Steven Rostedt wrote:

>> Here the same thing but without cmpxchg(). _If_ after an increment the
>> value is negative then we take slowpath. Otherwise we have the lock.
> 
> OK, so I need to make it so it can nest with trylock. I have to look at
> the patch again because it has been a while.

I have reverted the patch and can confirm that cpufreq works again.

I did some testing on vanilla and -RT:
- down_read(l) + down_read(l)
  this triggers a lockdep warning about a possible deadlock the lock is
  obtained.

- down_read(l) + down_read_trylock()
  this passes without a warning.

So I think we good now.

> An RW sem must not do two down_read()s on the same lock (it's fine for
> a trylock if it has a fail safe for it). The reason is, the second
> down_read() will block if there's a writer waiting. Thus you are
> guaranteed a deadlock if you have the lock for read, a write comes in
> and waits, and you grab the RW sem again, because it will block, and
> the writer is waiting for the reader to release. Thus you have a
> deadlock.

I fully understand. However nesting is allowed according to the code in
vanilla and now again in -RT. Lockdep complains properly so we should
catch people doing it wrong in both trees.

> I'll have to revisit this. I also need to revisit the multi readers
> (although Thomas hates it, but he even admitted there's a better way to
> do it. Now only if I could remember what that was ;-)

Okay. For now I keep the revert since it looks sane and simple.

> 
> Thanks,
> 
> -- Steve

Sebastian