v3:
- Add a new patch which adds a master LOCK_DEBUGGING option to turn
on all lock debugging.
v2:
- Fix typo in up_read_non_owner().
This patchset enhances lock debugging by adding rwsem lock/unlock
mismatches checking and a master config option to turn on all lock
debugging.
Waiman Long (2):
locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches
locking/debug: Add a master lock debugging switch
kernel/locking/rwsem.c | 4 ++++
kernel/locking/rwsem.h | 8 +++++++-
lib/Kconfig.debug | 39 ++++++++++++++++++++++++++++++++++-----
3 files changed, 45 insertions(+), 6 deletions(-)
--
1.8.3.1
For a rwsem, locking can either be exclusive or shared. The corresponding
exclusive or shared unlock must be used. Otherwise, the protected data
structures may get corrupted or the lock may be in an inconsistent state.
In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
is added which can be enabled to look for such mismatches and print
warnings that that happens.
Signed-off-by: Waiman Long <[email protected]>
---
kernel/locking/rwsem.c | 4 ++++
kernel/locking/rwsem.h | 8 +++++++-
lib/Kconfig.debug | 7 +++++++
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index f549c55..30465a2 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -117,6 +117,7 @@ int down_write_trylock(struct rw_semaphore *sem)
void up_read(struct rw_semaphore *sem)
{
rwsem_release(&sem->dep_map, 1, _RET_IP_);
+ DEBUG_RWSEMS_WARN_ON(sem->owner != RWSEM_READER_OWNED);
__up_read(sem);
}
@@ -129,6 +130,7 @@ void up_read(struct rw_semaphore *sem)
void up_write(struct rw_semaphore *sem)
{
rwsem_release(&sem->dep_map, 1, _RET_IP_);
+ DEBUG_RWSEMS_WARN_ON(sem->owner != current);
rwsem_clear_owner(sem);
__up_write(sem);
@@ -142,6 +144,7 @@ void up_write(struct rw_semaphore *sem)
void downgrade_write(struct rw_semaphore *sem)
{
lock_downgrade(&sem->dep_map, _RET_IP_);
+ DEBUG_RWSEMS_WARN_ON(sem->owner != current);
rwsem_set_reader_owned(sem);
__downgrade_write(sem);
@@ -211,6 +214,7 @@ int __sched down_write_killable_nested(struct rw_semaphore *sem, int subclass)
void up_read_non_owner(struct rw_semaphore *sem)
{
+ DEBUG_RWSEMS_WARN_ON(sem->owner != RWSEM_READER_OWNED);
__up_read(sem);
}
diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index a883b8f..563a7bc 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -16,6 +16,12 @@
*/
#define RWSEM_READER_OWNED ((struct task_struct *)1UL)
+#ifdef CONFIG_DEBUG_RWSEMS
+#define DEBUG_RWSEMS_WARN_ON(c) DEBUG_LOCKS_WARN_ON(c)
+#else
+#define DEBUG_RWSEMS_WARN_ON(c)
+#endif
+
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
/*
* All writes to owner are protected by WRITE_ONCE() to make sure that
@@ -41,7 +47,7 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
* do a write to the rwsem cacheline when it is really necessary
* to minimize cacheline contention.
*/
- if (sem->owner != RWSEM_READER_OWNED)
+ if (READ_ONCE(sem->owner) != RWSEM_READER_OWNED)
WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64155e3..0958192 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
even a debug kernel. If you are a driver writer, enable it. If
you are a distro, do not.
+config DEBUG_RWSEMS
+ bool "RW Semaphore debugging: basic checks"
+ depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
+ help
+ This feature allows mismatched rw semaphore locks and unlocks
+ to be detected and reported.
+
config DEBUG_LOCK_ALLOC
bool "Lock debugging: detect incorrect freeing of live locks"
depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
--
1.8.3.1
Add a new master LOCK_DEBUGGING Kconfig option to turn on all the lock
debugging options except the selftests and the torture tests.
Signed-off-by: Waiman Long <[email protected]>
---
lib/Kconfig.debug | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 0958192..817d78d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1034,6 +1034,28 @@ config DEBUG_PREEMPT
menu "Lock Debugging (spinlocks, mutexes, etc...)"
+config LOCK_DEBUGGING_SUPPORT
+ bool
+ depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ default y
+
+config LOCK_DEBUGGING
+ bool "Turn on all lock debugging"
+ depends on DEBUG_KERNEL
+ select DEBUG_SPINLOCK
+ select DEBUG_MUTEXES
+ select DEBUG_RT_MUTEXES if RT_MUTEXES
+ select DEBUG_RWSEMS if RWSEM_SPIN_ON_OWNER
+ select DEBUG_WW_MUTEX_SLOWPATH if LOCK_DEBUGGING_SUPPORT
+ select DEBUG_LOCK_ALLOC if LOCK_DEBUGGING_SUPPORT
+ select PROVE_LOCKING if LOCK_DEBUGGING_SUPPORT
+ select LOCK_STAT if LOCK_DEBUGGING_SUPPORT
+ select DEBUG_LOCKDEP if LOCK_DEBUGGING_SUPPORT
+ select DEBUG_ATOMIC_SLEEP
+ help
+ This turns on all the lock debugging code in the kernel except
+ the selftests and the torture tests.
+
config DEBUG_RT_MUTEXES
bool "RT Mutex debugging, deadlock detection"
depends on DEBUG_KERNEL && RT_MUTEXES
@@ -1060,7 +1082,7 @@ config DEBUG_MUTEXES
config DEBUG_WW_MUTEX_SLOWPATH
bool "Wait/wound mutex debugging: Slowpath testing"
- depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
select DEBUG_LOCK_ALLOC
select DEBUG_SPINLOCK
select DEBUG_MUTEXES
@@ -1084,7 +1106,7 @@ config DEBUG_RWSEMS
config DEBUG_LOCK_ALLOC
bool "Lock debugging: detect incorrect freeing of live locks"
- depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
select DEBUG_SPINLOCK
select DEBUG_MUTEXES
select DEBUG_RT_MUTEXES if RT_MUTEXES
@@ -1099,7 +1121,7 @@ config DEBUG_LOCK_ALLOC
config PROVE_LOCKING
bool "Lock debugging: prove locking correctness"
- depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
select LOCKDEP
select DEBUG_SPINLOCK
select DEBUG_MUTEXES
@@ -1143,7 +1165,7 @@ config PROVE_LOCKING
config LOCKDEP
bool
- depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
select STACKTRACE
select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !SCORE && !X86
select KALLSYMS
@@ -1154,7 +1176,7 @@ config LOCKDEP_SMALL
config LOCK_STAT
bool "Lock usage statistics"
- depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+ depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
select LOCKDEP
select DEBUG_SPINLOCK
select DEBUG_MUTEXES
--
1.8.3.1
On Tue, 27 Mar 2018, Waiman Long wrote:
>Add a new master LOCK_DEBUGGING Kconfig option to turn on all the lock
>debugging options except the selftests and the torture tests.
For what purpose?
I'm not sure we want yet another config debug option. These are all expert
level configuration and honestly I would rather such users _think_ about
what options they are enabling instead of giving them a large hammer.
If enough load, this can also make boxes completely locked up for any sort of
testing/debugging. I vote no.
Also, CI efforts such 0-day already have the necessary scripting to cover the
debug options that make sense.
Thanks,
Davidlohr
On 03/27/2018 07:13 PM, Davidlohr Bueso wrote:
> On Tue, 27 Mar 2018, Waiman Long wrote:
>
>> Add a new master LOCK_DEBUGGING Kconfig option to turn on all the lock
>> debugging options except the selftests and the torture tests.
>
> For what purpose?
>
> I'm not sure we want yet another config debug option. These are all
> expert
> level configuration and honestly I would rather such users _think_ about
> what options they are enabling instead of giving them a large hammer.
>
> If enough load, this can also make boxes completely locked up for any
> sort of
> testing/debugging. I vote no.
>
> Also, CI efforts such 0-day already have the necessary scripting to
> cover the
> debug options that make sense.
>
> Thanks,
> Davidlohr
I think I misunderstood what the expectation is. I will send out an
updated patch to correct that.
Thanks,
Longman