[PATCH v4 0/2] locking/rwsem: Fix DEBUG_RWSEM warning from thaw_super()

[PATCH v4 0/2] locking/rwsem: Fix DEBUG_RWSEM warning from thaw_super()

[Waiman Long]

v4:
 - Repurpose bit 0 of the owner field as the unknown owner bit.
 - Incorporate comments from reviewers.

v3:
 - Modify patch 1 to not expose any new rwsem owner related function.
 - Modify patch 2 to make percpu_rwsem_release() and
   percpu_rwsem_acquire() set the owner field directly, if applicable.

This patchset aims to fix the DEBUG_RWSEM warning in the filesystem
freezing/thawing code. A new macro RWSEM_OWNER_UNKNOWN (-1) is exposed in
the linux/rwsem.h file to indicate that a rwsem is currently owned by an
unknown writer. Other than that, there is no externally visible changes.

The new RWSEM_OWNER_UNKNOWN macro has no dependency on internal rwsem
header. What is important is that bit 1 of the owner field is set which
is the marker used by the internal rwsem code to determine if the owner
is unknown or not.

The original code of clearing the owner field in percpu_rwsem_release()
isn't correct as a NULL owner field should correspond to an unlocked
rwsem which is not the case here. In addition, optimistic spinning
won't be stopped by a NULL owner value.

Testings are welcome.

Waiman Long (2):
  locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag
  locking/percpu-rwsem: Annotate rwsem ownership transfer by setting
    RWSEM_OWNER_UNKNOWN

 include/linux/percpu-rwsem.h |  6 +++++-
 include/linux/rwsem.h        |  6 ++++++
 kernel/locking/rwsem-xadd.c  | 19 +++++++++----------
 kernel/locking/rwsem.c       |  2 --
 kernel/locking/rwsem.h       | 30 +++++++++++++++++++++---------
 5 files changed, 41 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Waiman Long]

There are use cases where a rwsem can be acquired by one task, but
released by another task. In thess cases, optimistic spinning may need
to be disabled.  One example will be the filesystem freeze/thaw code
where the task that freezes the filesystem will acquire a write lock
on a rwsem and then un-owns it before returning to userspace. Later on,
another task will come along, acquire the ownership, thaw the filesystem
and release the rwsem.

Bit 0 of the owner field was used to designate that it is a reader
owned rwsem. It is now repurposed to mean that the owner of the rwsem
is not known. If only bit 0 is set, the rwsem is reader owned. If bit
0 and other bits are set, it is writer owned with an unknown owner.
One such value for the latter case is (-1L). So we can set owner to 1 for
reader-owned, -1 for writer-owned. The owner is unknown in both cases.

To handle transfer of rwsem ownership, the higher level code should
set the owner field to -1 to indicate a write-locked rwsem with unknown
owner.  Optimistic spinning will be disabled in this case.

Once the higher level code figures who the new owner is, it can then
set the owner field accordingly.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/rwsem-xadd.c | 17 +++++++----------
 kernel/locking/rwsem.c      |  2 --
 kernel/locking/rwsem.h      | 30 +++++++++++++++++++++---------
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index e795908..604d247 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -357,11 +357,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 
 	rcu_read_lock();
 	owner = READ_ONCE(sem->owner);
-	if (!rwsem_owner_is_writer(owner)) {
-		/*
-		 * Don't spin if the rwsem is readers owned.
-		 */
-		ret = !rwsem_owner_is_reader(owner);
+	if (!owner || !is_rwsem_owner_spinnable(owner)) {
+		ret = !owner;	/* !owner is spinnable */
 		goto done;
 	}
 
@@ -382,11 +379,11 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner = READ_ONCE(sem->owner);
 
-	if (!rwsem_owner_is_writer(owner))
-		goto out;
+	if (!is_rwsem_owner_spinnable(owner))
+		return false;
 
 	rcu_read_lock();
-	while (sem->owner == owner) {
+	while (owner && (READ_ONCE(sem->owner) == owner)) {
 		/*
 		 * Ensure we emit the owner->on_cpu, dereference _after_
 		 * checking sem->owner still matches owner, if that fails,
@@ -408,12 +405,12 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 		cpu_relax();
 	}
 	rcu_read_unlock();
-out:
+
 	/*
 	 * If there is a new owner or the owner is not set, we continue
 	 * spinning.
 	 */
-	return !rwsem_owner_is_reader(READ_ONCE(sem->owner));
+	return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
 }
 
 static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 30465a2..bc1e507 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -221,5 +221,3 @@ void up_read_non_owner(struct rw_semaphore *sem)
 EXPORT_SYMBOL(up_read_non_owner);
 
 #endif
-
-
diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index a17cba8..b9d0e72 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -1,20 +1,24 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * The owner field of the rw_semaphore structure will be set to
- * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear
+ * RWSEM_READER_OWNED when a reader grabs the lock. A writer will clear
  * the owner field when it unlocks. A reader, on the other hand, will
  * not touch the owner field when it unlocks.
  *
- * In essence, the owner field now has the following 3 states:
+ * In essence, the owner field now has the following 4 states:
  *  1) 0
  *     - lock is free or the owner hasn't set the field yet
  *  2) RWSEM_READER_OWNED
  *     - lock is currently or previously owned by readers (lock is free
  *       or not set by owner yet)
- *  3) Other non-zero value
- *     - a writer owns the lock
+ *  3) RWSEM_ANONYMOUSLY_OWNED bit set with some other bits set as well
+ *     - lock is owned by an anonymous writer, so spinning on the lock
+ *       owner should be disabled.
+ *  4) Other non-zero value
+ *     - a writer owns the lock and other writers can spin on the lock owner.
  */
-#define RWSEM_READER_OWNED	((struct task_struct *)1UL)
+#define RWSEM_ANONYMOUSLY_OWNED	(1UL << 0)
+#define RWSEM_READER_OWNED	((struct task_struct *)RWSEM_ANONYMOUSLY_OWNED)
 
 #ifdef CONFIG_DEBUG_RWSEMS
 # define DEBUG_RWSEMS_WARN_ON(c)	DEBUG_LOCKS_WARN_ON(c)
@@ -51,14 +55,22 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
 		WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
 }
 
-static inline bool rwsem_owner_is_writer(struct task_struct *owner)
+/*
+ * Return true if the a rwsem waiter can spin on the rwsem's owner
+ * and steal the lock, i.e. the lock is not anonymously owned.
+ * N.B. !owner is considered spinnable.
+ */
+static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
 {
-	return owner && owner != RWSEM_READER_OWNED;
+	return !((unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED);
 }
 
-static inline bool rwsem_owner_is_reader(struct task_struct *owner)
+/*
+ * Return true if rwsem is owned by an anonymous writer or readers.
+ */
+static inline bool rwsem_has_anonymous_owner(struct task_struct *owner)
 {
-	return owner == RWSEM_READER_OWNED;
+	return (unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED;
 }
 #else
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
-- 
1.8.3.1

[PATCH v4 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN

[Waiman Long]

The filesystem freezing code needs to transfer ownership of a rwsem
embedded in a percpu-rwsem from the task that does the freezing to
another one that does the thawing by calling percpu_rwsem_release()
after freezing and percpu_rwsem_acquire() before thawing.

However, the new rwsem debug code runs afoul with this scheme by warning
that the task that releases the rwsem isn't the one that acquires it.

[   20.302978] ------------[ cut here ]------------
[   20.305016] DEBUG_LOCKS_WARN_ON(sem->owner != get_current())
[   20.305029] WARNING: CPU: 1 PID: 1401 at
/home/amir/build/src/linux/kernel/locking/rwsem.c:133 up_write+0x59/0x79
[   20.311252] CPU: 1 PID: 1401 Comm: fsfreeze Not tainted 4.17.0-rc3-xfstests-00049-g39e47bf59eb3 #3276
[   20.314808] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[   20.318403] RIP: 0010:up_write+0x59/0x79
[   20.320928] RSP: 0018:ffffc90000717e48 EFLAGS: 00010286
[   20.322955] RAX: 0000000000000030 RBX: ffff880078f1c680 RCX: ffff880078e42200
[   20.325665] RDX: ffffffff810cc9c1 RSI: 0000000000000001 RDI: 0000000000000202
[   20.328844] RBP: ffffc90000717e80 R08: 0000000000000001 R09: 0000000000000001
[   20.332340] R10: ffffc90000717c58 R11: ffffffff836807ad R12: ffff880078f1c388
[   20.335095] R13: ffff880078a8b980 R14: 0000000000000000 R15: 00000000fffffff7
[   20.338009] FS:  00007fb61ca42700(0000) GS:ffff88007f400000(0000) knlGS:0000000000000000
[   20.341423] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   20.343772] CR2: 00007fb61c559b30 CR3: 0000000078da6000 CR4: 00000000000006e0
[   20.346463] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   20.349201] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   20.351960] Call Trace:
[   20.352911]  percpu_up_write+0x1f/0x28
[   20.354344]  thaw_super_locked+0xdf/0x120
[   20.355944]  do_vfs_ioctl+0x270/0x5f1
[   20.357390]  ? __se_sys_newfstat+0x2e/0x39
[   20.358969]  ? entry_SYSCALL_64_after_hwframe+0x59/0xbe
[   20.360991]  ksys_ioctl+0x52/0x71
[   20.362384]  __x64_sys_ioctl+0x16/0x19
[   20.363702]  do_syscall_64+0x5d/0x167
[   20.365099]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

To work properly with the rwsem debug code, we need to annotate that the
rwsem ownership is unknown during the tranfer period until a brave soul
comes forward to acquire the ownership. During that period, optimistic
spinning will be disabled.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/percpu-rwsem.h | 6 +++++-
 include/linux/rwsem.h        | 6 ++++++
 kernel/locking/rwsem-xadd.c  | 2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index b1f37a8..79b99d6 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -133,7 +133,7 @@ static inline void percpu_rwsem_release(struct percpu_rw_semaphore *sem,
 	lock_release(&sem->rw_sem.dep_map, 1, ip);
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	if (!read)
-		sem->rw_sem.owner = NULL;
+		sem->rw_sem.owner = RWSEM_OWNER_UNKNOWN;
 #endif
 }
 
@@ -141,6 +141,10 @@ static inline void percpu_rwsem_acquire(struct percpu_rw_semaphore *sem,
 					bool read, unsigned long ip)
 {
 	lock_acquire(&sem->rw_sem.dep_map, 0, 1, read, 1, NULL, ip);
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
+	if (!read)
+		sem->rw_sem.owner = current;
+#endif
 }
 
 #endif
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 56707d5..ab93b6e 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -44,6 +44,12 @@ struct rw_semaphore {
 #endif
 };
 
+/*
+ * Setting bit 0 of the owner field with other non-zero bits will indicate
+ * that the rwsem is writer-owned with an unknown owner.
+ */
+#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1L)
+
 extern struct rw_semaphore *rwsem_down_read_failed(struct rw_semaphore *sem);
 extern struct rw_semaphore *rwsem_down_read_failed_killable(struct rw_semaphore *sem);
 extern struct rw_semaphore *rwsem_down_write_failed(struct rw_semaphore *sem);
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 604d247..a903367 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -352,6 +352,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	struct task_struct *owner;
 	bool ret = true;
 
+	BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN));
+
 	if (need_resched())
 		return false;
 
-- 
1.8.3.1

Re: [PATCH v4 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN

[Amir Goldstein]

On Wed, May 16, 2018 at 12:49 AM, Waiman Long <longman@redhat.com> wrote:
> The filesystem freezing code needs to transfer ownership of a rwsem
> embedded in a percpu-rwsem from the task that does the freezing to
> another one that does the thawing by calling percpu_rwsem_release()
> after freezing and percpu_rwsem_acquire() before thawing.
>
> However, the new rwsem debug code runs afoul with this scheme by warning
> that the task that releases the rwsem isn't the one that acquires it.
>
> [   20.302978] ------------[ cut here ]------------
> [   20.305016] DEBUG_LOCKS_WARN_ON(sem->owner != get_current())
> [   20.305029] WARNING: CPU: 1 PID: 1401 at
> /home/amir/build/src/linux/kernel/locking/rwsem.c:133 up_write+0x59/0x79
> [   20.311252] CPU: 1 PID: 1401 Comm: fsfreeze Not tainted 4.17.0-rc3-xfstests-00049-g39e47bf59eb3 #3276
> [   20.314808] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> [   20.318403] RIP: 0010:up_write+0x59/0x79
> [   20.320928] RSP: 0018:ffffc90000717e48 EFLAGS: 00010286
> [   20.322955] RAX: 0000000000000030 RBX: ffff880078f1c680 RCX: ffff880078e42200
> [   20.325665] RDX: ffffffff810cc9c1 RSI: 0000000000000001 RDI: 0000000000000202
> [   20.328844] RBP: ffffc90000717e80 R08: 0000000000000001 R09: 0000000000000001
> [   20.332340] R10: ffffc90000717c58 R11: ffffffff836807ad R12: ffff880078f1c388
> [   20.335095] R13: ffff880078a8b980 R14: 0000000000000000 R15: 00000000fffffff7
> [   20.338009] FS:  00007fb61ca42700(0000) GS:ffff88007f400000(0000) knlGS:0000000000000000
> [   20.341423] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   20.343772] CR2: 00007fb61c559b30 CR3: 0000000078da6000 CR4: 00000000000006e0
> [   20.346463] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   20.349201] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   20.351960] Call Trace:
> [   20.352911]  percpu_up_write+0x1f/0x28
> [   20.354344]  thaw_super_locked+0xdf/0x120
> [   20.355944]  do_vfs_ioctl+0x270/0x5f1
> [   20.357390]  ? __se_sys_newfstat+0x2e/0x39
> [   20.358969]  ? entry_SYSCALL_64_after_hwframe+0x59/0xbe
> [   20.360991]  ksys_ioctl+0x52/0x71
> [   20.362384]  __x64_sys_ioctl+0x16/0x19
> [   20.363702]  do_syscall_64+0x5d/0x167
> [   20.365099]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> To work properly with the rwsem debug code, we need to annotate that the
> rwsem ownership is unknown during the tranfer period until a brave soul
> comes forward to acquire the ownership. During that period, optimistic
> spinning will be disabled.
>
> Signed-off-by: Waiman Long <longman@redhat.com>

Looks good and tested

Thanks,
Amir.

> ---
>  include/linux/percpu-rwsem.h | 6 +++++-
>  include/linux/rwsem.h        | 6 ++++++
>  kernel/locking/rwsem-xadd.c  | 2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
> index b1f37a8..79b99d6 100644
> --- a/include/linux/percpu-rwsem.h
> +++ b/include/linux/percpu-rwsem.h
> @@ -133,7 +133,7 @@ static inline void percpu_rwsem_release(struct percpu_rw_semaphore *sem,
>         lock_release(&sem->rw_sem.dep_map, 1, ip);
>  #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
>         if (!read)
> -               sem->rw_sem.owner = NULL;
> +               sem->rw_sem.owner = RWSEM_OWNER_UNKNOWN;
>  #endif
>  }
>
> @@ -141,6 +141,10 @@ static inline void percpu_rwsem_acquire(struct percpu_rw_semaphore *sem,
>                                         bool read, unsigned long ip)
>  {
>         lock_acquire(&sem->rw_sem.dep_map, 0, 1, read, 1, NULL, ip);
> +#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
> +       if (!read)
> +               sem->rw_sem.owner = current;
> +#endif
>  }
>
>  #endif
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index 56707d5..ab93b6e 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -44,6 +44,12 @@ struct rw_semaphore {
>  #endif
>  };
>
> +/*
> + * Setting bit 0 of the owner field with other non-zero bits will indicate
> + * that the rwsem is writer-owned with an unknown owner.
> + */
> +#define RWSEM_OWNER_UNKNOWN    ((struct task_struct *)-1L)
> +
>  extern struct rw_semaphore *rwsem_down_read_failed(struct rw_semaphore *sem);
>  extern struct rw_semaphore *rwsem_down_read_failed_killable(struct rw_semaphore *sem);
>  extern struct rw_semaphore *rwsem_down_write_failed(struct rw_semaphore *sem);
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index 604d247..a903367 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -352,6 +352,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>         struct task_struct *owner;
>         bool ret = true;
>
> +       BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN));
> +
>         if (need_resched())
>                 return false;
>
> --
> 1.8.3.1
>

[tip:locking/urgent] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[tip-bot for Waiman Long]

Commit-ID:  d7d760efad70c7a030725499bf9f342f04af24dd
Gitweb:     https://git.kernel.org/tip/d7d760efad70c7a030725499bf9f342f04af24dd
Author:     Waiman Long <longman@redhat.com>
AuthorDate: Tue, 15 May 2018 17:49:50 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 May 2018 11:45:15 +0200

locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

There are use cases where a rwsem can be acquired by one task, but
released by another task. In thess cases, optimistic spinning may need
to be disabled.  One example will be the filesystem freeze/thaw code
where the task that freezes the filesystem will acquire a write lock
on a rwsem and then un-owns it before returning to userspace. Later on,
another task will come along, acquire the ownership, thaw the filesystem
and release the rwsem.

Bit 0 of the owner field was used to designate that it is a reader
owned rwsem. It is now repurposed to mean that the owner of the rwsem
is not known. If only bit 0 is set, the rwsem is reader owned. If bit
0 and other bits are set, it is writer owned with an unknown owner.
One such value for the latter case is (-1L). So we can set owner to 1 for
reader-owned, -1 for writer-owned. The owner is unknown in both cases.

To handle transfer of rwsem ownership, the higher level code should
set the owner field to -1 to indicate a write-locked rwsem with unknown
owner.  Optimistic spinning will be disabled in this case.

Once the higher level code figures who the new owner is, it can then
set the owner field accordingly.

Tested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jan Kara <jack@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Theodore Y. Ts'o <tytso@mit.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-fsdevel@vger.kernel.org
Link: http://lkml.kernel.org/r/1526420991-21213-2-git-send-email-longman@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/rwsem-xadd.c | 17 +++++++----------
 kernel/locking/rwsem.c      |  2 --
 kernel/locking/rwsem.h      | 30 +++++++++++++++++++++---------
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index e795908f3607..604d247ea8c3 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -357,11 +357,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 
 	rcu_read_lock();
 	owner = READ_ONCE(sem->owner);
-	if (!rwsem_owner_is_writer(owner)) {
-		/*
-		 * Don't spin if the rwsem is readers owned.
-		 */
-		ret = !rwsem_owner_is_reader(owner);
+	if (!owner || !is_rwsem_owner_spinnable(owner)) {
+		ret = !owner;	/* !owner is spinnable */
 		goto done;
 	}
 
@@ -382,11 +379,11 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner = READ_ONCE(sem->owner);
 
-	if (!rwsem_owner_is_writer(owner))
-		goto out;
+	if (!is_rwsem_owner_spinnable(owner))
+		return false;
 
 	rcu_read_lock();
-	while (sem->owner == owner) {
+	while (owner && (READ_ONCE(sem->owner) == owner)) {
 		/*
 		 * Ensure we emit the owner->on_cpu, dereference _after_
 		 * checking sem->owner still matches owner, if that fails,
@@ -408,12 +405,12 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 		cpu_relax();
 	}
 	rcu_read_unlock();
-out:
+
 	/*
 	 * If there is a new owner or the owner is not set, we continue
 	 * spinning.
 	 */
-	return !rwsem_owner_is_reader(READ_ONCE(sem->owner));
+	return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
 }
 
 static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 30465a2f2b6c..bc1e507be9ff 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -221,5 +221,3 @@ void up_read_non_owner(struct rw_semaphore *sem)
 EXPORT_SYMBOL(up_read_non_owner);
 
 #endif
-
-
diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index a17cba8d94bb..b9d0e72aa80f 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -1,20 +1,24 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * The owner field of the rw_semaphore structure will be set to
- * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear
+ * RWSEM_READER_OWNED when a reader grabs the lock. A writer will clear
  * the owner field when it unlocks. A reader, on the other hand, will
  * not touch the owner field when it unlocks.
  *
- * In essence, the owner field now has the following 3 states:
+ * In essence, the owner field now has the following 4 states:
  *  1) 0
  *     - lock is free or the owner hasn't set the field yet
  *  2) RWSEM_READER_OWNED
  *     - lock is currently or previously owned by readers (lock is free
  *       or not set by owner yet)
- *  3) Other non-zero value
- *     - a writer owns the lock
+ *  3) RWSEM_ANONYMOUSLY_OWNED bit set with some other bits set as well
+ *     - lock is owned by an anonymous writer, so spinning on the lock
+ *       owner should be disabled.
+ *  4) Other non-zero value
+ *     - a writer owns the lock and other writers can spin on the lock owner.
  */
-#define RWSEM_READER_OWNED	((struct task_struct *)1UL)
+#define RWSEM_ANONYMOUSLY_OWNED	(1UL << 0)
+#define RWSEM_READER_OWNED	((struct task_struct *)RWSEM_ANONYMOUSLY_OWNED)
 
 #ifdef CONFIG_DEBUG_RWSEMS
 # define DEBUG_RWSEMS_WARN_ON(c)	DEBUG_LOCKS_WARN_ON(c)
@@ -51,14 +55,22 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
 		WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
 }
 
-static inline bool rwsem_owner_is_writer(struct task_struct *owner)
+/*
+ * Return true if the a rwsem waiter can spin on the rwsem's owner
+ * and steal the lock, i.e. the lock is not anonymously owned.
+ * N.B. !owner is considered spinnable.
+ */
+static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
 {
-	return owner && owner != RWSEM_READER_OWNED;
+	return !((unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED);
 }
 
-static inline bool rwsem_owner_is_reader(struct task_struct *owner)
+/*
+ * Return true if rwsem is owned by an anonymous writer or readers.
+ */
+static inline bool rwsem_has_anonymous_owner(struct task_struct *owner)
 {
-	return owner == RWSEM_READER_OWNED;
+	return (unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED;
 }
 #else
 static inline void rwsem_set_owner(struct rw_semaphore *sem)

[tip:locking/urgent] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN

[tip-bot for Waiman Long]

Commit-ID:  5a817641f68a6399a5fac8b7d2da67a73698ffed
Gitweb:     https://git.kernel.org/tip/5a817641f68a6399a5fac8b7d2da67a73698ffed
Author:     Waiman Long <longman@redhat.com>
AuthorDate: Tue, 15 May 2018 17:49:51 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 May 2018 11:45:16 +0200

locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN

The filesystem freezing code needs to transfer ownership of a rwsem
embedded in a percpu-rwsem from the task that does the freezing to
another one that does the thawing by calling percpu_rwsem_release()
after freezing and percpu_rwsem_acquire() before thawing.

However, the new rwsem debug code runs afoul with this scheme by warning
that the task that releases the rwsem isn't the one that acquires it,
as reported by Amir Goldstein:

  DEBUG_LOCKS_WARN_ON(sem->owner != get_current())
  WARNING: CPU: 1 PID: 1401 at /home/amir/build/src/linux/kernel/locking/rwsem.c:133 up_write+0x59/0x79

  Call Trace:
   percpu_up_write+0x1f/0x28
   thaw_super_locked+0xdf/0x120
   do_vfs_ioctl+0x270/0x5f1
   ksys_ioctl+0x52/0x71
   __x64_sys_ioctl+0x16/0x19
   do_syscall_64+0x5d/0x167
   entry_SYSCALL_64_after_hwframe+0x49/0xbe

To work properly with the rwsem debug code, we need to annotate that the
rwsem ownership is unknown during the tranfer period until a brave soul
comes forward to acquire the ownership. During that period, optimistic
spinning will be disabled.

Reported-by: Amir Goldstein <amir73il@gmail.com>
Tested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jan Kara <jack@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Theodore Y. Ts'o <tytso@mit.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-fsdevel@vger.kernel.org
Link: http://lkml.kernel.org/r/1526420991-21213-3-git-send-email-longman@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/percpu-rwsem.h | 6 +++++-
 include/linux/rwsem.h        | 6 ++++++
 kernel/locking/rwsem-xadd.c  | 2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index b1f37a89e368..79b99d653e03 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -133,7 +133,7 @@ static inline void percpu_rwsem_release(struct percpu_rw_semaphore *sem,
 	lock_release(&sem->rw_sem.dep_map, 1, ip);
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	if (!read)
-		sem->rw_sem.owner = NULL;
+		sem->rw_sem.owner = RWSEM_OWNER_UNKNOWN;
 #endif
 }
 
@@ -141,6 +141,10 @@ static inline void percpu_rwsem_acquire(struct percpu_rw_semaphore *sem,
 					bool read, unsigned long ip)
 {
 	lock_acquire(&sem->rw_sem.dep_map, 0, 1, read, 1, NULL, ip);
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
+	if (!read)
+		sem->rw_sem.owner = current;
+#endif
 }
 
 #endif
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 56707d5ff6ad..ab93b6eae696 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -44,6 +44,12 @@ struct rw_semaphore {
 #endif
 };
 
+/*
+ * Setting bit 0 of the owner field with other non-zero bits will indicate
+ * that the rwsem is writer-owned with an unknown owner.
+ */
+#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1L)
+
 extern struct rw_semaphore *rwsem_down_read_failed(struct rw_semaphore *sem);
 extern struct rw_semaphore *rwsem_down_read_failed_killable(struct rw_semaphore *sem);
 extern struct rw_semaphore *rwsem_down_write_failed(struct rw_semaphore *sem);
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 604d247ea8c3..a90336779375 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -352,6 +352,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	struct task_struct *owner;
 	bool ret = true;
 
+	BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN));
+
 	if (need_resched())
 		return false;
 

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Oleg Nesterov]

On 05/15, Waiman Long wrote:
>
> There are use cases where a rwsem can be acquired by one task, but
> released by another task. In thess cases, optimistic spinning may need
> to be disabled.  One example will be the filesystem freeze/thaw code

You do not read my emails ;)

Let me repeat once again that in this particular case the writer will
never spin because of owner == NULL. freeze_super() checks SB_UNFROZEN
under sb->s_umount and only then calls sb_wait_write(). IOW, sb_wait_write()
can only be called when this rwsem was already released by the previous
writer.

I am not arguing with this change, percpu_rwsem_release/acquire may have
another user sometime, but the changelog is not accurate.

> +static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
>  {
> -	return owner && owner != RWSEM_READER_OWNED;
> +	return !((unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED);
>  }

Perhaps you should add __attribute__(aligned) to struct rw_semaphore then...

I don't think it is really needed, but see the comment under struct address_space.

Oleg.

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Peter Zijlstra]

On Wed, May 16, 2018 at 12:48:30PM +0200, Oleg Nesterov wrote:
> > +static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
> >  {
> > -	return owner && owner != RWSEM_READER_OWNED;
> > +	return !((unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED);
> >  }
> 
> Perhaps you should add __attribute__(aligned) to struct rw_semaphore then...
> 
> I don't think it is really needed, but see the comment under struct address_space.

Luckily we just dropped CRIS support, but yeah, who knows if some other
dodgy arch also doesn't properly align things.

>From a quick test, m68k is the only odd one, it seems to align pointers
on 2 bytes.

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Matthew Wilcox]

On Tue, May 15, 2018 at 05:49:50PM -0400, Waiman Long wrote:
> @@ -357,11 +357,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>  
>  	rcu_read_lock();
>  	owner = READ_ONCE(sem->owner);
> -	if (!rwsem_owner_is_writer(owner)) {
> -		/*
> -		 * Don't spin if the rwsem is readers owned.
> -		 */
> -		ret = !rwsem_owner_is_reader(owner);
> +	if (!owner || !is_rwsem_owner_spinnable(owner)) {
> +		ret = !owner;	/* !owner is spinnable */
>  		goto done;
>  	}

This is confusingly written.  I think you mean ...

	if (!owner)
		goto done;
	if (!is_rwsem_owner_spinnable(owner)) {
		ret = false;
		goto done;
	}

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Waiman Long]

On 05/16/2018 06:48 AM, Oleg Nesterov wrote:
> On 05/15, Waiman Long wrote:
>> There are use cases where a rwsem can be acquired by one task, but
>> released by another task. In thess cases, optimistic spinning may need
>> to be disabled.  One example will be the filesystem freeze/thaw code
> You do not read my emails ;)
>
> Let me repeat once again that in this particular case the writer will
> never spin because of owner == NULL. freeze_super() checks SB_UNFROZEN
> under sb->s_umount and only then calls sb_wait_write(). IOW, sb_wait_write()
> can only be called when this rwsem was already released by the previous
> writer.
>
> I am not arguing with this change, percpu_rwsem_release/acquire may have
> another user sometime, but the changelog is not accurate.

I know the change may not be necessary in this particular case, but it
is a correctness issue. Optimistic spinning should be disabled when the
exact time delay between percpu_rwsem_release() and
percpu_rwsem_acquire() is indeterminate even though no one is supposed
to spin on the rwsem during that time.

If we don't do that now, we may forget this issue when some other use
cases show up or we extend rwsem to do reader optimistic spinning, for
instance. So it is better to address that now than debugging the same
issue again in the future.

Cheers,
Longman

Re: [PATCH v4 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN

[Waiman Long]

On 05/16/2018 01:37 AM, Amir Goldstein wrote:
> On Wed, May 16, 2018 at 12:49 AM, Waiman Long <longman@redhat.com> wrote:
>> The filesystem freezing code needs to transfer ownership of a rwsem
>> embedded in a percpu-rwsem from the task that does the freezing to
>> another one that does the thawing by calling percpu_rwsem_release()
>> after freezing and percpu_rwsem_acquire() before thawing.
>>
>> However, the new rwsem debug code runs afoul with this scheme by warning
>> that the task that releases the rwsem isn't the one that acquires it.
>>
>> [   20.302978] ------------[ cut here ]------------
>> [   20.305016] DEBUG_LOCKS_WARN_ON(sem->owner != get_current())
>> [   20.305029] WARNING: CPU: 1 PID: 1401 at
>> /home/amir/build/src/linux/kernel/locking/rwsem.c:133 up_write+0x59/0x79
>> [   20.311252] CPU: 1 PID: 1401 Comm: fsfreeze Not tainted 4.17.0-rc3-xfstests-00049-g39e47bf59eb3 #3276
>> [   20.314808] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
>> [   20.318403] RIP: 0010:up_write+0x59/0x79
>> [   20.320928] RSP: 0018:ffffc90000717e48 EFLAGS: 00010286
>> [   20.322955] RAX: 0000000000000030 RBX: ffff880078f1c680 RCX: ffff880078e42200
>> [   20.325665] RDX: ffffffff810cc9c1 RSI: 0000000000000001 RDI: 0000000000000202
>> [   20.328844] RBP: ffffc90000717e80 R08: 0000000000000001 R09: 0000000000000001
>> [   20.332340] R10: ffffc90000717c58 R11: ffffffff836807ad R12: ffff880078f1c388
>> [   20.335095] R13: ffff880078a8b980 R14: 0000000000000000 R15: 00000000fffffff7
>> [   20.338009] FS:  00007fb61ca42700(0000) GS:ffff88007f400000(0000) knlGS:0000000000000000
>> [   20.341423] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   20.343772] CR2: 00007fb61c559b30 CR3: 0000000078da6000 CR4: 00000000000006e0
>> [   20.346463] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [   20.349201] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> [   20.351960] Call Trace:
>> [   20.352911]  percpu_up_write+0x1f/0x28
>> [   20.354344]  thaw_super_locked+0xdf/0x120
>> [   20.355944]  do_vfs_ioctl+0x270/0x5f1
>> [   20.357390]  ? __se_sys_newfstat+0x2e/0x39
>> [   20.358969]  ? entry_SYSCALL_64_after_hwframe+0x59/0xbe
>> [   20.360991]  ksys_ioctl+0x52/0x71
>> [   20.362384]  __x64_sys_ioctl+0x16/0x19
>> [   20.363702]  do_syscall_64+0x5d/0x167
>> [   20.365099]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> To work properly with the rwsem debug code, we need to annotate that the
>> rwsem ownership is unknown during the tranfer period until a brave soul
>> comes forward to acquire the ownership. During that period, optimistic
>> spinning will be disabled.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> Looks good and tested
>
> Thanks,
> Amir.
>
Thanks for the testing.

Cheers,
Longman

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Oleg Nesterov]

On 05/16, Waiman Long wrote:
>
> On 05/16/2018 06:48 AM, Oleg Nesterov wrote:
> > On 05/15, Waiman Long wrote:
> >> There are use cases where a rwsem can be acquired by one task, but
> >> released by another task. In thess cases, optimistic spinning may need
> >> to be disabled.  One example will be the filesystem freeze/thaw code
> > You do not read my emails ;)
> >
> > Let me repeat once again that in this particular case the writer will
> > never spin because of owner == NULL. freeze_super() checks SB_UNFROZEN
> > under sb->s_umount and only then calls sb_wait_write(). IOW, sb_wait_write()
> > can only be called when this rwsem was already released by the previous
> > writer.
> >
> > I am not arguing with this change, percpu_rwsem_release/acquire may have
> > another user sometime, but the changelog is not accurate.
>
> I know the change may not be necessary in this particular case, but it
> is a correctness issue.

Really? I mean, performance-wise the unnecessary spinning is obviously bad,
but why it is a correctness issue?

And how this differs from the case when down_write() is preempted right
before rwsem_set_owner() ?

> Optimistic spinning should be disabled when the
> exact time delay between percpu_rwsem_release() and
> percpu_rwsem_acquire() is indeterminate even though no one is supposed
> to spin on the rwsem during that time.
>
> If we don't do that now, we may forget this issue when

See above, I never argued with this change. Just the changelog looks as if
we already have this issue in freeze/thaw code, this is not true.

Oleg.

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Ingo Molnar]

* Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, May 15, 2018 at 05:49:50PM -0400, Waiman Long wrote:
> > @@ -357,11 +357,8 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
> >  
> >  	rcu_read_lock();
> >  	owner = READ_ONCE(sem->owner);
> > -	if (!rwsem_owner_is_writer(owner)) {
> > -		/*
> > -		 * Don't spin if the rwsem is readers owned.
> > -		 */
> > -		ret = !rwsem_owner_is_reader(owner);
> > +	if (!owner || !is_rwsem_owner_spinnable(owner)) {
> > +		ret = !owner;	/* !owner is spinnable */
> >  		goto done;
> >  	}
> 
> This is confusingly written.  I think you mean ...
> 
> 	if (!owner)
> 		goto done;
> 	if (!is_rwsem_owner_spinnable(owner)) {
> 		ret = false;
> 		goto done;
> 	}

Yes, that's cleaner. Waiman, mind sending a followup patch that cleans this up?

Thanks,

	Ingo

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Oleg Nesterov]

On 05/18, Ingo Molnar wrote:
>
>
> * Matthew Wilcox <willy@infradead.org> wrote:
>
> > This is confusingly written.  I think you mean ...
> >
> > 	if (!owner)
> > 		goto done;
> > 	if (!is_rwsem_owner_spinnable(owner)) {
> > 		ret = false;
> > 		goto done;
> > 	}
>
> Yes, that's cleaner. Waiman, mind sending a followup patch that cleans this up?

Or simply

	static inline bool owner_on_cpu(struct task_struct *owner)
	{
		return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
	}

	static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
	{
		struct task_struct *owner;
		bool ret = true;

		if (need_resched())
			return false;

		rcu_read_lock();
		owner = READ_ONCE(sem->owner);
		if (owner) {
			ret = is_rwsem_owner_spinnable(owner) &&
			      owner_on_cpu(owner);
		}
		rcu_read_unlock();
		return ret;
	}

note that rwsem_spin_on_owner() can use the new owner_on_cpu() helper too,

		if (need_resched() || !owner_on_cpu(owner)) {
			rcu_read_unlock();
			return false;
		}

looks a bit better than the current code:

		if (!owner->on_cpu || need_resched() ||
				vcpu_is_preempted(task_cpu(owner))) {
			rcu_read_unlock();
			return false;
		}

Oleg.

Re: [PATCH v4 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/18, Ingo Molnar wrote:
> >
> >
> > * Matthew Wilcox <willy@infradead.org> wrote:
> >
> > > This is confusingly written.  I think you mean ...
> > >
> > > 	if (!owner)
> > > 		goto done;
> > > 	if (!is_rwsem_owner_spinnable(owner)) {
> > > 		ret = false;
> > > 		goto done;
> > > 	}
> >
> > Yes, that's cleaner. Waiman, mind sending a followup patch that cleans this up?
> 
> Or simply
> 
> 	static inline bool owner_on_cpu(struct task_struct *owner)
> 	{
> 		return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
> 	}
> 
> 	static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
> 	{
> 		struct task_struct *owner;
> 		bool ret = true;
> 
> 		if (need_resched())
> 			return false;
> 
> 		rcu_read_lock();
> 		owner = READ_ONCE(sem->owner);
> 		if (owner) {
> 			ret = is_rwsem_owner_spinnable(owner) &&
> 			      owner_on_cpu(owner);
> 		}
> 		rcu_read_unlock();
> 		return ret;
> 	}
> 
> note that rwsem_spin_on_owner() can use the new owner_on_cpu() helper too,
> 
> 		if (need_resched() || !owner_on_cpu(owner)) {
> 			rcu_read_unlock();
> 			return false;
> 		}
> 
> looks a bit better than the current code:
> 
> 		if (!owner->on_cpu || need_resched() ||
> 				vcpu_is_preempted(task_cpu(owner))) {
> 			rcu_read_unlock();
> 			return false;
> 		}
> 
> Oleg.

That looks good to me too - mind sending a patch on top of latest -tip?

Thanks,

	Ingo

[PATCH] locking/rwsem: simplify the is-owner-spinnable checks

[Oleg Nesterov]

Add the trivial owner_on_cpu() helper for rwsem_can_spin_on_owner() and
rwsem_spin_on_owner(), it also allows to make rwsem_can_spin_on_owner()
a bit more clear.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/locking/rwsem-xadd.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index a903367..3064c50 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -347,6 +347,15 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 	}
 }
 
+static inline bool owner_on_cpu(struct task_struct *owner)
+{
+	/*
+	 * As lock holder preemption issue, we both skip spinning if
+	 * task is not on cpu or its cpu is preempted
+	 */
+	return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+}
+
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
@@ -359,17 +368,10 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 
 	rcu_read_lock();
 	owner = READ_ONCE(sem->owner);
-	if (!owner || !is_rwsem_owner_spinnable(owner)) {
-		ret = !owner;	/* !owner is spinnable */
-		goto done;
+	if (owner) {
+		ret = is_rwsem_owner_spinnable(owner) &&
+		      owner_on_cpu(owner);
 	}
-
-	/*
-	 * As lock holder preemption issue, we both skip spinning if task is not
-	 * on cpu or its cpu is preempted
-	 */
-	ret = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
-done:
 	rcu_read_unlock();
 	return ret;
 }
@@ -398,8 +400,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 		 * abort spinning when need_resched or owner is not running or
 		 * owner's cpu is preempted.
 		 */
-		if (!owner->on_cpu || need_resched() ||
-				vcpu_is_preempted(task_cpu(owner))) {
+		if (need_resched() || !owner_on_cpu(owner)) {
 			rcu_read_unlock();
 			return false;
 		}
-- 
2.5.0

Re: [PATCH] locking/rwsem: simplify the is-owner-spinnable checks

[Waiman Long]

On 05/18/2018 12:55 PM, Oleg Nesterov wrote:
> Add the trivial owner_on_cpu() helper for rwsem_can_spin_on_owner() and
> rwsem_spin_on_owner(), it also allows to make rwsem_can_spin_on_owner()
> a bit more clear.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/locking/rwsem-xadd.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index a903367..3064c50 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -347,6 +347,15 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
>  	}
>  }
>  
> +static inline bool owner_on_cpu(struct task_struct *owner)
> +{
> +	/*
> +	 * As lock holder preemption issue, we both skip spinning if
> +	 * task is not on cpu or its cpu is preempted
> +	 */
> +	return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
> +}
> +
>  static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>  {
>  	struct task_struct *owner;
> @@ -359,17 +368,10 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>  
>  	rcu_read_lock();
>  	owner = READ_ONCE(sem->owner);
> -	if (!owner || !is_rwsem_owner_spinnable(owner)) {
> -		ret = !owner;	/* !owner is spinnable */
> -		goto done;
> +	if (owner) {
> +		ret = is_rwsem_owner_spinnable(owner) &&
> +		      owner_on_cpu(owner);
>  	}
> -
> -	/*
> -	 * As lock holder preemption issue, we both skip spinning if task is not
> -	 * on cpu or its cpu is preempted
> -	 */
> -	ret = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
> -done:
>  	rcu_read_unlock();
>  	return ret;
>  }
> @@ -398,8 +400,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
>  		 * abort spinning when need_resched or owner is not running or
>  		 * owner's cpu is preempted.
>  		 */
> -		if (!owner->on_cpu || need_resched() ||
> -				vcpu_is_preempted(task_cpu(owner))) {
> +		if (need_resched() || !owner_on_cpu(owner)) {
>  			rcu_read_unlock();
>  			return false;
>  		}

Acked-by: Waiman Long <longman@redhat.com>

[tip:locking/core] locking/rwsem: Simplify the is-owner-spinnable checks

[tip-bot for Oleg Nesterov]

Commit-ID:  1b22fc609cecd1b16c4a015e1a6b3c9717484e3a
Gitweb:     https://git.kernel.org/tip/1b22fc609cecd1b16c4a015e1a6b3c9717484e3a
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Fri, 18 May 2018 18:55:35 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 25 May 2018 08:11:47 +0200

locking/rwsem: Simplify the is-owner-spinnable checks

Add the trivial owner_on_cpu() helper for rwsem_can_spin_on_owner() and
rwsem_spin_on_owner(), it also allows to make rwsem_can_spin_on_owner()
a bit more clear.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Waiman Long <longman@redhat.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jan Kara <jack@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Theodore Y. Ts'o <tytso@mit.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20180518165534.GA22348@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/rwsem-xadd.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index a90336779375..3064c50e181e 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -347,6 +347,15 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 	}
 }
 
+static inline bool owner_on_cpu(struct task_struct *owner)
+{
+	/*
+	 * As lock holder preemption issue, we both skip spinning if
+	 * task is not on cpu or its cpu is preempted
+	 */
+	return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+}
+
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
@@ -359,17 +368,10 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 
 	rcu_read_lock();
 	owner = READ_ONCE(sem->owner);
-	if (!owner || !is_rwsem_owner_spinnable(owner)) {
-		ret = !owner;	/* !owner is spinnable */
-		goto done;
+	if (owner) {
+		ret = is_rwsem_owner_spinnable(owner) &&
+		      owner_on_cpu(owner);
 	}
-
-	/*
-	 * As lock holder preemption issue, we both skip spinning if task is not
-	 * on cpu or its cpu is preempted
-	 */
-	ret = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
-done:
 	rcu_read_unlock();
 	return ret;
 }
@@ -398,8 +400,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
 		 * abort spinning when need_resched or owner is not running or
 		 * owner's cpu is preempted.
 		 */
-		if (!owner->on_cpu || need_resched() ||
-				vcpu_is_preempted(task_cpu(owner))) {
+		if (need_resched() || !owner_on_cpu(owner)) {
 			rcu_read_unlock();
 			return false;
 		}