kernel: Rewrite KDEBUG_RW_LOCK_DEBUG.

Previously this just turned the rw_lock into the equivalent of a
recursive_lock, which meant that reader vs. writer assertions
were of no use.

Now, we have a per-thread static array which stores the held read
locks, allowing ASSERT_READ_LOCKED_RW_LOCK to work properly,
and allowing multiple readers to be active at a time.

This probably should still remain disabled even on nightly builds,
but at least it's much more useful as a debugging tool than it was
beforehand.

Change-Id: I386b2bc2ada8df42f4ab11a05563ef22af58e77f

headers/private/kernel/lock.h

@@ -72,23 +72,27 @@ typedef struct rw_lock {
 #if KDEBUG
 #	define KDEBUG_RW_LOCK_DEBUG 0
 		// Define to 1 if you want to use ASSERT_READ_LOCKED_RW_LOCK().
-		// The rw_lock will just behave like a recursive locker then.
 #	define ASSERT_LOCKED_RECURSIVE(r) \
 		{ ASSERT(find_thread(NULL) == (r)->lock.holder); }
 #	define ASSERT_LOCKED_MUTEX(m) { ASSERT(find_thread(NULL) == (m)->holder); }
 #	define ASSERT_WRITE_LOCKED_RW_LOCK(l) \
 		{ ASSERT(find_thread(NULL) == (l)->holder); }
 #	if KDEBUG_RW_LOCK_DEBUG
+		extern bool _rw_lock_is_read_locked(rw_lock* lock);
 #		define ASSERT_READ_LOCKED_RW_LOCK(l) \
-			{ ASSERT(find_thread(NULL) == (l)->holder); }
+			{ ASSERT_PRINT(_rw_lock_is_read_locked(l), "rwlock %p", l); }
+#		define ASSERT_UNLOCKED_RW_LOCK(l) \
+			{ ASSERT_PRINT(!_rw_lock_is_read_locked(l), "rwlock %p", l); }
 #	else
 #		define ASSERT_READ_LOCKED_RW_LOCK(l) do {} while (false)
+#		define ASSERT_UNLOCKED_RW_LOCK(l)	 do {} while (false)
 #	endif
 #else
 #	define ASSERT_LOCKED_RECURSIVE(r)		do {} while (false)
 #	define ASSERT_LOCKED_MUTEX(m)			do {} while (false)
 #	define ASSERT_WRITE_LOCKED_RW_LOCK(m)	do {} while (false)
 #	define ASSERT_READ_LOCKED_RW_LOCK(l)	do {} while (false)
+#	define ASSERT_UNLOCKED_RW_LOCK(l)		do {} while (false)
 #endif
 
 
@@ -180,7 +184,7 @@ static inline status_t
 rw_lock_read_lock(rw_lock* lock)
 {
 #if KDEBUG_RW_LOCK_DEBUG
-	return rw_lock_write_lock(lock);
+	return _rw_lock_read_lock(lock);
 #else
 	int32 oldCount = atomic_add(&lock->count, 1);
 	if (oldCount >= RW_LOCK_WRITER_COUNT_BASE)
@@ -195,7 +199,7 @@ rw_lock_read_lock_with_timeout(rw_lock* lock, uint32 timeoutFlags,
 	bigtime_t timeout)
 {
 #if KDEBUG_RW_LOCK_DEBUG
-	return mutex_lock_with_timeout(lock, timeoutFlags, timeout);
+	return _rw_lock_read_lock_with_timeout(lock, timeoutFlags, timeout);
 #else
 	int32 oldCount = atomic_add(&lock->count, 1);
 	if (oldCount >= RW_LOCK_WRITER_COUNT_BASE)
@@ -209,7 +213,7 @@ static inline void
 rw_lock_read_unlock(rw_lock* lock)
 {
 #if KDEBUG_RW_LOCK_DEBUG
-	rw_lock_write_unlock(lock);
+	_rw_lock_read_unlock(lock);
 #else
 	int32 oldCount = atomic_add(&lock->count, -1);
 	if (oldCount >= RW_LOCK_WRITER_COUNT_BASE)

headers/private/kernel/thread_types.h

@@ -547,6 +547,10 @@ struct Thread : TeamThreadIteratorEntry<thread_id>, KernelReferenceable {
 	void			(*post_interrupt_callback)(void*);
 	void*			post_interrupt_data;
 
+#if KDEBUG_RW_LOCK_DEBUG
+	rw_lock*		held_read_locks[64] = {}; // only modified by this thread
+#endif
+
 	// architecture dependent section
 	struct arch_thread arch_info;
 

src/system/kernel/locks/lock.cpp

@@ -441,7 +441,53 @@ rw_lock_destroy(rw_lock* lock)
 }
 
 
-#if !KDEBUG_RW_LOCK_DEBUG
+#if KDEBUG_RW_LOCK_DEBUG
+
+bool
+_rw_lock_is_read_locked(rw_lock* lock)
+{
+	if (lock->holder == thread_get_current_thread_id())
+		return true;
+
+	Thread* thread = thread_get_current_thread();
+	for (size_t i = 0; i < B_COUNT_OF(Thread::held_read_locks); i++) {
+		if (thread->held_read_locks[i] == lock)
+			return true;
+	}
+	return false;
+}
+
+
+static void
+_rw_lock_set_read_locked(rw_lock* lock)
+{
+	Thread* thread = thread_get_current_thread();
+	for (size_t i = 0; i < B_COUNT_OF(Thread::held_read_locks); i++) {
+		if (thread->held_read_locks[i] != NULL)
+			continue;
+
+		thread->held_read_locks[i] = lock;
+		return;
+	}
+	panic("too many read locks!");
+}
+
+
+static void
+_rw_lock_unset_read_locked(rw_lock* lock)
+{
+	Thread* thread = thread_get_current_thread();
+	for (size_t i = 0; i < B_COUNT_OF(Thread::held_read_locks); i++) {
+		if (thread->held_read_locks[i] != lock)
+			continue;
+
+		thread->held_read_locks[i] = NULL;
+		return;
+	}
+}
+
+#endif
+
 
 status_t
 _rw_lock_read_lock(rw_lock* lock)
@@ -452,6 +498,14 @@ _rw_lock_read_lock(rw_lock* lock)
 			lock);
 	}
 #endif
+#if KDEBUG_RW_LOCK_DEBUG
+	int32 oldCount = atomic_add(&lock->count, 1);
+	if (oldCount < RW_LOCK_WRITER_COUNT_BASE) {
+		ASSERT_UNLOCKED_RW_LOCK(lock);
+		_rw_lock_set_read_locked(lock);
+		return B_OK;
+	}
+#endif
 
 	InterruptsSpinLocker locker(lock->lock);
 
@@ -461,6 +515,8 @@ _rw_lock_read_lock(rw_lock* lock)
 		return B_OK;
 	}
 
+	ASSERT_UNLOCKED_RW_LOCK(lock);
+
 	// The writer that originally had the lock when we called atomic_add() might
 	// already have gone and another writer could have overtaken us. In this
 	// case the original writer set pending_readers, so we know that we don't
@@ -471,13 +527,23 @@ _rw_lock_read_lock(rw_lock* lock)
 		if (lock->count >= RW_LOCK_WRITER_COUNT_BASE)
 			lock->active_readers++;
 
+#if KDEBUG_RW_LOCK_DEBUG
+		_rw_lock_set_read_locked(lock);
+#endif
 		return B_OK;
 	}
 
 	ASSERT(lock->count >= RW_LOCK_WRITER_COUNT_BASE);
 
 	// we need to wait
-	return rw_lock_wait(lock, false, locker);
+	status_t status = rw_lock_wait(lock, false, locker);
+
+#if KDEBUG_RW_LOCK_DEBUG
+	if (status == B_OK)
+		_rw_lock_set_read_locked(lock);
+#endif
+
+	return status;
 }
 
 
@@ -491,6 +557,14 @@ _rw_lock_read_lock_with_timeout(rw_lock* lock, uint32 timeoutFlags,
 			"disabled for lock %p", lock);
 	}
 #endif
+#if KDEBUG_RW_LOCK_DEBUG
+	int32 oldCount = atomic_add(&lock->count, 1);
+	if (oldCount < RW_LOCK_WRITER_COUNT_BASE) {
+		ASSERT_UNLOCKED_RW_LOCK(lock);
+		_rw_lock_set_read_locked(lock);
+		return B_OK;
+	}
+#endif
 
 	InterruptsSpinLocker locker(lock->lock);
 
@@ -500,6 +574,8 @@ _rw_lock_read_lock_with_timeout(rw_lock* lock, uint32 timeoutFlags,
 		return B_OK;
 	}
 
+	ASSERT_UNLOCKED_RW_LOCK(lock);
+
 	// The writer that originally had the lock when we called atomic_add() might
 	// already have gone and another writer could have overtaken us. In this
 	// case the original writer set pending_readers, so we know that we don't
@@ -510,6 +586,9 @@ _rw_lock_read_lock_with_timeout(rw_lock* lock, uint32 timeoutFlags,
 		if (lock->count >= RW_LOCK_WRITER_COUNT_BASE)
 			lock->active_readers++;
 
+#if KDEBUG_RW_LOCK_DEBUG
+		_rw_lock_set_read_locked(lock);
+#endif
 		return B_OK;
 	}
 
@@ -538,6 +617,9 @@ _rw_lock_read_lock_with_timeout(rw_lock* lock, uint32 timeoutFlags,
 	if (error == B_OK || waiter.thread == NULL) {
 		// We were unblocked successfully -- potentially our unblocker overtook
 		// us after we already failed. In either case, we've got the lock, now.
+#if KDEBUG_RW_LOCK_DEBUG
+		_rw_lock_set_read_locked(lock);
+#endif
 		return B_OK;
 	}
 
@@ -586,6 +668,14 @@ _rw_lock_read_unlock(rw_lock* lock)
 		return;
 	}
 
+#if KDEBUG_RW_LOCK_DEBUG
+	_rw_lock_unset_read_locked(lock);
+
+	int32 oldCount = atomic_add(&lock->count, -1);
+	if (oldCount < RW_LOCK_WRITER_COUNT_BASE)
+		return;
+#endif
+
 	if (--lock->active_readers > 0)
 		return;
 
@@ -598,8 +688,6 @@ _rw_lock_read_unlock(rw_lock* lock)
 	rw_lock_unblock(lock);
 }
 
-#endif	// !KDEBUG_RW_LOCK_DEBUG
-
 
 status_t
 rw_lock_write_lock(rw_lock* lock)
@@ -621,6 +709,8 @@ rw_lock_write_lock(rw_lock* lock)
 		return B_OK;
 	}
 
+	ASSERT_UNLOCKED_RW_LOCK(lock);
+
 	// announce our claim
 	int32 oldCount = atomic_add(&lock->count, RW_LOCK_WRITER_COUNT_BASE);
 
@@ -668,6 +758,11 @@ _rw_lock_write_unlock(rw_lock* lock)
 	lock->holder = -1;
 	lock->owner_count = 0;
 
+#if KDEBUG_RW_LOCK_DEBUG
+	if (readerCount != 0)
+		_rw_lock_set_read_locked(lock);
+#endif
+
 	int32 oldCount = atomic_add(&lock->count, -RW_LOCK_WRITER_COUNT_BASE);
 	oldCount -= RW_LOCK_WRITER_COUNT_BASE;