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

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

[Waiman Long]

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        |  5 +++++
 kernel/locking/rwsem-xadd.c  | 17 ++++++++---------
 kernel/locking/rwsem.c       |  5 ++---
 kernel/locking/rwsem.h       | 33 ++++++++++++++++++++++++---------
 5 files changed, 44 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v3 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 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.

To handle such transfer of ownership, a new RWSEM_ANONYMOUSLY_OWNED
flag can now be set in the owner field of the rwsem when the ownership
is to be tranfered to indicate that the rwsem is owned an anonymous
writer. Since the actual owner is not known after the flag is set,
optimistic spinning on the rwsem will be disabled.

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      |  5 ++---
 kernel/locking/rwsem.h      | 33 ++++++++++++++++++++++++---------
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index e795908..a27dbb4 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,8 +379,10 @@ 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 (!owner)
+		return true;
+	else if (!is_rwsem_owner_spinnable(owner))
+		return false;
 
 	rcu_read_lock();
 	while (sem->owner == owner) {
@@ -408,12 +407,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..b7208e1 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -130,7 +130,8 @@ 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);
+	DEBUG_RWSEMS_WARN_ON((sem->owner != current) &&
+			     !rwsem_has_anonymous_owner(sem->owner));
 
 	rwsem_clear_owner(sem);
 	__up_write(sem);
@@ -221,5 +222,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..d035e8c 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -1,20 +1,27 @@
 /* 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
+ *     - 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_READER_OWNED		(1UL << 0)
+#define __RWSEM_ANONYMOUSLY_OWNED	(1UL << 1)
+#define RWSEM_NOSPIN_MASK	(__RWSEM_READER_OWNED|__RWSEM_ANONYMOUSLY_OWNED)
+#define RWSEM_READER_OWNED	((struct task_struct *)__RWSEM_READER_OWNED)
+#define WSEM_ANONYMOUSLY_OWNED	((struct task_struct *)__RWSEM_ANONYMOUSLY_OWNED)
 
 #ifdef CONFIG_DEBUG_RWSEMS
 # define DEBUG_RWSEMS_WARN_ON(c)	DEBUG_LOCKS_WARN_ON(c)
@@ -51,14 +58,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 lock is owned by an anonymous writer.
+ */
+static inline bool rwsem_has_anonymous_owner(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 the a rwsem waiter can spin on the rwsem's owner
+ * and steal the lock.
+ * N.B. !owner is considered spinnable.
+ */
+static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
 {
-	return owner == RWSEM_READER_OWNED;
+	return !((unsigned long)owner & RWSEM_NOSPIN_MASK);
 }
 #else
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
-- 
1.8.3.1

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
> +#define WSEM_ANONYMOUSLY_OWNED	((struct task_struct *)__RWSEM_ANONYMOUSLY_OWNED)

typoed and unused..

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

[Waiman Long]

On 05/15/2018 01:46 PM, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
>> +#define WSEM_ANONYMOUSLY_OWNED	((struct task_struct *)__RWSEM_ANONYMOUSLY_OWNED)
> typoed and unused..

Sorry about the typo. I know it is not used. I include it for
completeness purpose. I will remove it in the next version.

Cheers,
Longman

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 30465a2..b7208e1 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -130,7 +130,8 @@ 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);
> +	DEBUG_RWSEMS_WARN_ON((sem->owner != current) &&
> +			     !rwsem_has_anonymous_owner(sem->owner));

Why? Don't we always do percpu_rwsem_acquire() before up?

>  
>  	rwsem_clear_owner(sem);
>  	__up_write(sem);

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

[Waiman Long]

On 05/15/2018 01:48 PM, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 30465a2..b7208e1 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -130,7 +130,8 @@ 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);
>> +	DEBUG_RWSEMS_WARN_ON((sem->owner != current) &&
>> +			     !rwsem_has_anonymous_owner(sem->owner));
> Why? Don't we always do percpu_rwsem_acquire() before up?
>

This is to allow an unlock to happen in the unknown owner state. Yes, it
is not necessary to fix the percpu-rwsem problem. It is there just in
case a rwsem will be released in that state in the future.

Cheers,
Longman

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 01:52:06PM -0400, Waiman Long wrote:
> On 05/15/2018 01:48 PM, Peter Zijlstra wrote:
> > On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
> >> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> >> index 30465a2..b7208e1 100644
> >> --- a/kernel/locking/rwsem.c
> >> +++ b/kernel/locking/rwsem.c
> >> @@ -130,7 +130,8 @@ 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);
> >> +	DEBUG_RWSEMS_WARN_ON((sem->owner != current) &&
> >> +			     !rwsem_has_anonymous_owner(sem->owner));
> > Why? Don't we always do percpu_rwsem_acquire() before up?
> >
> 
> This is to allow an unlock to happen in the unknown owner state. Yes, it
> is not necessary to fix the percpu-rwsem problem. It is there just in
> case a rwsem will be released in that state in the future.

Let's not allow that until there's a very good reason for it.

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

[Waiman Long]

On 05/15/2018 01:55 PM, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 01:52:06PM -0400, Waiman Long wrote:
>> On 05/15/2018 01:48 PM, Peter Zijlstra wrote:
>>> On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
>>>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>>>> index 30465a2..b7208e1 100644
>>>> --- a/kernel/locking/rwsem.c
>>>> +++ b/kernel/locking/rwsem.c
>>>> @@ -130,7 +130,8 @@ 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);
>>>> +	DEBUG_RWSEMS_WARN_ON((sem->owner != current) &&
>>>> +			     !rwsem_has_anonymous_owner(sem->owner));
>>> Why? Don't we always do percpu_rwsem_acquire() before up?
>>>
>> This is to allow an unlock to happen in the unknown owner state. Yes, it
>> is not necessary to fix the percpu-rwsem problem. It is there just in
>> case a rwsem will be released in that state in the future.
> Let's not allow that until there's a very good reason for it.

Sure. I can remove that.

Cheers,
Longman

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 01:38:03PM -0400, Waiman Long wrote:
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index e795908..a27dbb4 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,8 +379,10 @@ 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 (!owner)
> +		return true;
> +	else if (!is_rwsem_owner_spinnable(owner))
> +		return false;

s/else //

>  	rcu_read_lock();
>  	while (sem->owner == owner) {
> @@ -408,12 +407,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));
>  }

The above two cases have explicit owner tests, this one doesn't.

Now I know it works, but maybe:

	return !owner || is_rwsem_owner_spinnable(owner);

is easier to read... dunno.

[PATCH v3 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        | 5 +++++
 2 files changed, 10 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..fc2ed27 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -44,6 +44,11 @@ struct rw_semaphore {
 #endif
 };
 
+/*
+ * Owner value to indicate the rwsem's owner is not currently known.
+ */
+#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
+
 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);
-- 
1.8.3.1

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> +/*
> + * Owner value to indicate the rwsem's owner is not currently known.
> + */
> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)

It might be nice to comment that this works and relies on having that
ANON_OWNER bit set.

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

[Waiman Long]

On 05/15/2018 01:58 PM, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>> +/*
>> + * Owner value to indicate the rwsem's owner is not currently known.
>> + */
>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> It might be nice to comment that this works and relies on having that
> ANON_OWNER bit set.

I am just trying not to expose internal working of rwsem, but I can
document that one of the bits is the real deal without specifying which one.

Cheers,
Longman

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

[Amir Goldstein]

On Tue, May 15, 2018 at 9:02 PM, Waiman Long <longman@redhat.com> wrote:
> On 05/15/2018 01:58 PM, Peter Zijlstra wrote:
>> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>>> +/*
>>> + * Owner value to indicate the rwsem's owner is not currently known.
>>> + */
>>> +#define RWSEM_OWNER_UNKNOWN ((struct task_struct *)-1)
>> It might be nice to comment that this works and relies on having that
>> ANON_OWNER bit set.
>
> I am just trying not to expose internal working of rwsem, but I can
> document that one of the bits is the real deal without specifying which one.
>

BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN))
inside rwsem.c and comment above to explain it.

Thanks,
Amir.

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

[Waiman Long]

On 05/15/2018 02:05 PM, Amir Goldstein wrote:
> On Tue, May 15, 2018 at 9:02 PM, Waiman Long <longman@redhat.com> wrote:
>> On 05/15/2018 01:58 PM, Peter Zijlstra wrote:
>>> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>>>> +/*
>>>> + * Owner value to indicate the rwsem's owner is not currently known.
>>>> + */
>>>> +#define RWSEM_OWNER_UNKNOWN ((struct task_struct *)-1)
>>> It might be nice to comment that this works and relies on having that
>>> ANON_OWNER bit set.
>> I am just trying not to expose internal working of rwsem, but I can
>> document that one of the bits is the real deal without specifying which one.
>>
> BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN))
> inside rwsem.c and comment above to explain it.

Yes, it is a good idea. I will do that.

-Longman

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 02:02:00PM -0400, Waiman Long wrote:
> On 05/15/2018 01:58 PM, Peter Zijlstra wrote:
> > On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> >> +/*
> >> + * Owner value to indicate the rwsem's owner is not currently known.
> >> + */
> >> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> > It might be nice to comment that this works and relies on having that
> > ANON_OWNER bit set.
> 
> I am just trying not to expose internal working of rwsem, but I can
> document that one of the bits is the real deal without specifying which one.

Thing is, you don't want someone changing this without knowing about
that one magic bit. It doesn't hurt to be explicit here.

Also, do we want -1L instead of a -1 literal?

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

[Waiman Long]

On 05/15/2018 02:05 PM, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 02:02:00PM -0400, Waiman Long wrote:
>> On 05/15/2018 01:58 PM, Peter Zijlstra wrote:
>>> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>>>> +/*
>>>> + * Owner value to indicate the rwsem's owner is not currently known.
>>>> + */
>>>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
>>> It might be nice to comment that this works and relies on having that
>>> ANON_OWNER bit set.
>> I am just trying not to expose internal working of rwsem, but I can
>> document that one of the bits is the real deal without specifying which one.
> Thing is, you don't want someone changing this without knowing about
> that one magic bit. It doesn't hurt to be explicit here.
>
> Also, do we want -1L instead of a -1 literal?
>
Yes, I should -1L instead.

Cheers,
Longman

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

[Matthew Wilcox]

On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> > +/*
> > + * Owner value to indicate the rwsem's owner is not currently known.
> > + */
> > +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> 
> It might be nice to comment that this works and relies on having that
> ANON_OWNER bit set.

I'd rather change the definition to be ((struct task_struct *)2)
otherwise this is both reader-owned and anonymously-owned which doesn't
make much sense.

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

[Peter Zijlstra]

On Tue, May 15, 2018 at 11:02:19AM -0700, Matthew Wilcox wrote:
> On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
> > On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> > > +/*
> > > + * Owner value to indicate the rwsem's owner is not currently known.
> > > + */
> > > +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> > 
> > It might be nice to comment that this works and relies on having that
> > ANON_OWNER bit set.
> 
> I'd rather change the definition to be ((struct task_struct *)2)
> otherwise this is both reader-owned and anonymously-owned which doesn't
> make much sense.

Works for me.

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

[Waiman Long]

On 05/15/2018 02:02 PM, Matthew Wilcox wrote:
> On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
>> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>>> +/*
>>> + * Owner value to indicate the rwsem's owner is not currently known.
>>> + */
>>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
>> It might be nice to comment that this works and relies on having that
>> ANON_OWNER bit set.
> I'd rather change the definition to be ((struct task_struct *)2)
> otherwise this is both reader-owned and anonymously-owned which doesn't
> make much sense.

Thinking about it a bit more. I can actually just use one special bit
(bit 0) to designate an unknown owner. So for a reader-owned lock, it is
just owner == 1 as the owners are unknown for a reader owned lock. For a
lock owned by an unknown writer, it is (owner & 1) && (owner != 1). That
will justify the use of -1L and save bit 1 for future extension.

Cheers,
Longman

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

[Matthew Wilcox]

On Tue, May 15, 2018 at 02:45:12PM -0400, Waiman Long wrote:
> On 05/15/2018 02:02 PM, Matthew Wilcox wrote:
> > On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
> >> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> >>> +/*
> >>> + * Owner value to indicate the rwsem's owner is not currently known.
> >>> + */
> >>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> >> It might be nice to comment that this works and relies on having that
> >> ANON_OWNER bit set.
> > I'd rather change the definition to be ((struct task_struct *)2)
> > otherwise this is both reader-owned and anonymously-owned which doesn't
> > make much sense.
> 
> Thinking about it a bit more. I can actually just use one special bit
> (bit 0) to designate an unknown owner. So for a reader-owned lock, it is
> just owner == 1 as the owners are unknown for a reader owned lock. For a
> lock owned by an unknown writer, it is (owner & 1) && (owner != 1). That
> will justify the use of -1L and save bit 1 for future extension.

To quote from your patch:

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

I'd leave these as 0, 1, 2, other.  It's not really worth messing with
testing bits.

Actually, if you change them to all be values -- s/NULL/RWSEM_NO_OWNER/

then you could define them as:

RWSEM_READER_OWNED	0
RWSEM_ANON_OWNED	1
RWSEM_NO_OWNER		2

and rwsem_should_spin() is just sem->owner > 1.

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

[Waiman Long]

On 05/15/2018 05:21 PM, Matthew Wilcox wrote:
> On Tue, May 15, 2018 at 02:45:12PM -0400, Waiman Long wrote:
>> On 05/15/2018 02:02 PM, Matthew Wilcox wrote:
>>> On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
>>>> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
>>>>> +/*
>>>>> + * Owner value to indicate the rwsem's owner is not currently known.
>>>>> + */
>>>>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
>>>> It might be nice to comment that this works and relies on having that
>>>> ANON_OWNER bit set.
>>> I'd rather change the definition to be ((struct task_struct *)2)
>>> otherwise this is both reader-owned and anonymously-owned which doesn't
>>> make much sense.
>> Thinking about it a bit more. I can actually just use one special bit
>> (bit 0) to designate an unknown owner. So for a reader-owned lock, it is
>> just owner == 1 as the owners are unknown for a reader owned lock. For a
>> lock owned by an unknown writer, it is (owner & 1) && (owner != 1). That
>> will justify the use of -1L and save bit 1 for future extension.
> To quote from your patch:
>
> - * 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
> + *     - 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.
>
> I'd leave these as 0, 1, 2, other.  It's not really worth messing with
> testing bits.
>
> Actually, if you change them to all be values -- s/NULL/RWSEM_NO_OWNER/
>
> then you could define them as:
>
> RWSEM_READER_OWNED	0
> RWSEM_ANON_OWNED	1
> RWSEM_NO_OWNER		2
>
> and rwsem_should_spin() is just sem->owner > 1.

I would like to have owner equal to NULL if it is not locked. If it is
locked, the owner can be used to get information about the owner. So I
am not sure your scheme will work.

Cheers,
Longman