* [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
* 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 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
* [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 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
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.