* [PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline
@ 2014-04-09 2:47 Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
0 siblings, 2 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-04-09 2:47 UTC (permalink / raw)
To: linux-kernel, linux-rt-users
Cc: Thomas Gleixner, Carsten Emde, Sebastian Andrzej Siewior,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
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(-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2014-04-09 2:47 [PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline Steven Rostedt
@ 2014-04-09 2:47 ` Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
1 sibling, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-04-09 2:47 UTC (permalink / raw)
To: linux-kernel, linux-rt-users
Cc: Thomas Gleixner, Carsten Emde, Sebastian Andrzej Siewior,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
[-- 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
@ 2014-04-09 2:47 ` Steven Rostedt
0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-04-09 2:47 UTC (permalink / raw)
To: linux-kernel, linux-rt-users
Cc: Thomas Gleixner, Carsten Emde, Sebastian Andrzej Siewior,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
[-- 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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()
2014-04-09 2:47 [PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline Steven Rostedt
@ 2014-04-09 2:47 ` Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
1 sibling, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-04-09 2:47 UTC (permalink / raw)
To: linux-kernel, linux-rt-users
Cc: Thomas Gleixner, Carsten Emde, Sebastian Andrzej Siewior,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
[-- 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()
@ 2014-04-09 2:47 ` Steven Rostedt
0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-04-09 2:47 UTC (permalink / raw)
To: linux-kernel, linux-rt-users
Cc: Thomas Gleixner, Carsten Emde, Sebastian Andrzej Siewior,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
[-- 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2014-04-09 2:47 ` Steven Rostedt
(?)
@ 2014-05-02 9:04 ` Sebastian Andrzej Siewior
-1 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2014-05-02 9:04 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()
2014-04-09 2:47 ` Steven Rostedt
(?)
@ 2014-05-02 9:08 ` Sebastian Andrzej Siewior
2014-05-02 13:12 ` Steven Rostedt
-1 siblings, 1 reply; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2014-05-02 9:08 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()
2014-05-02 9:08 ` Sebastian Andrzej Siewior
@ 2014-05-02 13:12 ` Steven Rostedt
2014-05-02 13:21 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-05-02 13:12 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init()
2014-05-02 13:12 ` Steven Rostedt
@ 2014-05-02 13:21 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2014-05-02 13:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2014-04-09 2:47 ` Steven Rostedt
(?)
(?)
@ 2015-02-18 19:57 ` Sebastian Andrzej Siewior
2015-02-18 20:13 ` Steven Rostedt
-1 siblings, 1 reply; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-02-18 19:57 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2015-02-18 19:57 ` Sebastian Andrzej Siewior
@ 2015-02-18 20:13 ` Steven Rostedt
2015-02-20 5:07 ` Jason Low
2015-02-25 12:15 ` Sebastian Andrzej Siewior
0 siblings, 2 replies; 13+ messages in thread
From: Steven Rostedt @ 2015-02-18 20:13 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2015-02-18 20:13 ` Steven Rostedt
@ 2015-02-20 5:07 ` Jason Low
2015-02-25 12:15 ` Sebastian Andrzej Siewior
1 sibling, 0 replies; 13+ messages in thread
From: Jason Low @ 2015-02-20 5:07 UTC (permalink / raw)
To: Steven Rostedt
Cc: Sebastian Andrzej Siewior, Linux Kernel Mailing List,
linux-rt-users, Thomas Gleixner, Carsten Emde, John Kacur,
Paul Gortmaker, Peter Zijlstra, H. Peter Anvin, Jason Low,
juerg.haefliger
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest
2015-02-18 20:13 ` Steven Rostedt
2015-02-20 5:07 ` Jason Low
@ 2015-02-25 12:15 ` Sebastian Andrzej Siewior
1 sibling, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-02-25 12:15 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-rt-users, Thomas Gleixner, Carsten Emde,
John Kacur, Paul Gortmaker, Peter Zijlstra, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-02-25 12:16 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-09 2:47 [PATCH RT 0/2] rwsem-rt: Make rwsem rt closer to mainline Steven Rostedt
2014-04-09 2:47 ` [PATCH RT 1/2] rwsem-rt: Do not allow readers to nest Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
2014-05-02 9:04 ` Sebastian Andrzej Siewior
2015-02-18 19:57 ` Sebastian Andrzej Siewior
2015-02-18 20:13 ` Steven Rostedt
2015-02-20 5:07 ` Jason Low
2015-02-25 12:15 ` Sebastian Andrzej Siewior
2014-04-09 2:47 ` [PATCH RT 2/2] rtmutex: Remove duplicate rt_mutex_init() Steven Rostedt
2014-04-09 2:47 ` Steven Rostedt
2014-05-02 9:08 ` Sebastian Andrzej Siewior
2014-05-02 13:12 ` Steven Rostedt
2014-05-02 13:21 ` Sebastian Andrzej Siewior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.