[PATCH] locking/rwsem: Make rwsem_is_contended() track status of OSQ

[PATCH] locking/rwsem: Make rwsem_is_contended() track status of OSQ

[Waiman Long]

It was found that the current rwsem_is_contended() function did not
look at the status of the OSQ and hence would miss waiters on OSQ. So
that function is now modified to look at the OSQ as well.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/rwsem.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index dd1d142..9cb64e3 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -71,8 +71,18 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
 
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 #define __RWSEM_OPT_INIT(lockname) , .osq = OSQ_LOCK_UNLOCKED, .owner = NULL
+
+static inline bool rwsem_osq_is_locked(struct rw_semaphore *sem)
+{
+	return osq_is_locked(&sem->osq);
+}
 #else
 #define __RWSEM_OPT_INIT(lockname)
+
+static inline bool rwsem_osq_is_locked(struct rw_semaphore *sem)
+{
+	return false;
+}
 #endif
 
 #define __RWSEM_INITIALIZER(name)				\
@@ -103,7 +113,7 @@ extern void __init_rwsem(struct rw_semaphore *sem, const char *name,
  */
 static inline int rwsem_is_contended(struct rw_semaphore *sem)
 {
-	return !list_empty(&sem->wait_list);
+	return !list_empty(&sem->wait_list) || rwsem_osq_is_locked(sem);
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH] locking/rwsem: Make rwsem_is_contended() track status of OSQ

[Peter Zijlstra]

On Tue, Mar 07, 2017 at 11:03:48AM -0500, Waiman Long wrote:
> It was found that the current rwsem_is_contended() function did not
> look at the status of the OSQ and hence would miss waiters on OSQ. So
> that function is now modified to look at the OSQ as well.

Ideally I'd kill the entire function.

	if (need_resched() ||
	    rwsem_is_contended(&fs_info->commit_root_sem)) {
		if (wakeup)
			caching_ctl->progress = last;
		btrfs_release_path(path);
		up_read(&fs_info->commit_root_sem);
		mutex_unlock(&caching_ctl->mutex);
		cond_resched();
		mutex_lock(&caching_ctl->mutex);
		down_read(&fs_info->commit_root_sem);
		goto next;
	}

is the only user of it in the entire tree and it makes no bloody sense
what so ever. rwsem is a preemptible lock after all.

Re: [PATCH] locking/rwsem: Make rwsem_is_contended() track status of OSQ

[Waiman Long]

On 03/07/2017 12:45 PM, Peter Zijlstra wrote:
> On Tue, Mar 07, 2017 at 11:03:48AM -0500, Waiman Long wrote:
>> It was found that the current rwsem_is_contended() function did not
>> look at the status of the OSQ and hence would miss waiters on OSQ. So
>> that function is now modified to look at the OSQ as well.
> Ideally I'd kill the entire function.
>
> 	if (need_resched() ||
> 	    rwsem_is_contended(&fs_info->commit_root_sem)) {
> 		if (wakeup)
> 			caching_ctl->progress = last;
> 		btrfs_release_path(path);
> 		up_read(&fs_info->commit_root_sem);
> 		mutex_unlock(&caching_ctl->mutex);
> 		cond_resched();
> 		mutex_lock(&caching_ctl->mutex);
> 		down_read(&fs_info->commit_root_sem);
> 		goto next;
> 	}
>
> is the only user of it in the entire tree and it makes no bloody sense
> what so ever. rwsem is a preemptible lock after all.

That works for me too. I do realize that there is only one user in the
kernel, but I am hesitant to change it as I am not familiar with that
piece of code.

Cheers,
Longman