[PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Zqiang]

When the lock owner is on CPU and not need resched, the current waiter
need to be checked, if it not longer top the waiter, stop spinning.

Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter lockless")
Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 v1->v2:
 Modify description information.

 kernel/locking/rtmutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 0c1f2e3f019a..8555c4efe97c 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1383,7 +1383,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 		 *  - the VCPU on which owner runs is preempted
 		 */
 		if (!owner_on_cpu(owner) || need_resched() ||
-		    rt_mutex_waiter_is_top_waiter(lock, waiter)) {
+		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
 			res = false;
 			break;
 		}
-- 
2.25.1

Re: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Thomas Gleixner]

Zqiang,

On Fri, Dec 17 2021 at 15:42, Zqiang wrote:
> When the lock owner is on CPU and not need resched, the current waiter
> need to be checked, if it not longer top the waiter, stop spinning.
>
> Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter lockless")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
>  v1->v2:
>  Modify description information.
>
>  kernel/locking/rtmutex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 0c1f2e3f019a..8555c4efe97c 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1383,7 +1383,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
>  		 *  - the VCPU on which owner runs is preempted
>  		 */
>  		if (!owner_on_cpu(owner) || need_resched() ||
> -		    rt_mutex_waiter_is_top_waiter(lock, waiter)) {
> +		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
>  			res = false;
>  			break;

good catch!

Though this does not apply because the condition is incomplete. You
somehow dropped this from the condition:

                   vcpu_is_preempted(task_cpu(owner))) 

Please make always sure that your patches apply against Linus tree
before sending them out.

Thanks,

        tglx

RE: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Zhang, Qiang1]

-----Original Message-----
From: Thomas Gleixner <tglx@linutronix.de> 
Sent: 2021年12月18日 4:53
To: Zhang, Qiang1 <qiang1.zhang@intel.com>; peterz@infradead.org; mingo@redhat.com; will@kernel.org; longman@redhat.com
Cc: linux-kernel@vger.kernel.org; Zhang, Qiang1 <qiang1.zhang@intel.com>
Subject: Re: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

Zqiang,

On Fri, Dec 17 2021 at 15:42, Zqiang wrote:
> When the lock owner is on CPU and not need resched, the current waiter
> need to be checked, if it not longer top the waiter, stop spinning.
>
> Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter lockless")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
>  v1->v2:
>  Modify description information.
>
>  kernel/locking/rtmutex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 0c1f2e3f019a..8555c4efe97c 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1383,7 +1383,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
>  		 *  - the VCPU on which owner runs is preempted
>  		 */
>  		if (!owner_on_cpu(owner) || need_resched() ||
> -		    rt_mutex_waiter_is_top_waiter(lock, waiter)) {
> +		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
>  			res = false;
>  			break;
>
>good catch!
>
>Though this does not apply because the condition is incomplete. You
>somehow dropped this from the condition:
>
>                   vcpu_is_preempted(task_cpu(owner))) 
>
>Please make always sure that your patches apply against Linus tree
>before sending them out.

This commit c0bed69daf4b ("locking: Make owner_on_cpu() into <linux/sched.h>")
make the following modifications in latest linux-next.

+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));
+}
+

Thanks

Zqiang

>
>Thanks,
>
>        tglx

RE: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Thomas Gleixner]

On Sat, Dec 18 2021 at 07:24, Qiang1 Zhang wrote:
> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de> 
> Sent: 2021年12月18日 4:53
> To: Zhang, Qiang1 <qiang1.zhang@intel.com>; peterz@infradead.org; mingo@redhat.com; will@kernel.org; longman@redhat.com
> Cc: linux-kernel@vger.kernel.org; Zhang, Qiang1 <qiang1.zhang@intel.com>
> Subject: Re: [PATCH v2] locking/rtmutex: Fix incorrect spinning
> condition

Can you please fix your mail client to do proper replies without copying
the mail headers into the message?

>>Though this does not apply because the condition is incomplete. You
>>somehow dropped this from the condition:
>>
>>                   vcpu_is_preempted(task_cpu(owner))) 
>>
>>Please make always sure that your patches apply against Linus tree
>>before sending them out.
>
> This commit c0bed69daf4b ("locking: Make owner_on_cpu() into <linux/sched.h>")
> make the following modifications in latest linux-next.
>
> +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));
> +}
> +

Fine, but then please tell against which tree/branch the patch is.

Thanks,

        tglx

RE: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Zhang, Qiang1]

>Can you please fix your mail client to do proper replies without copying the mail headers into the message?

I have been fix it

>>Though this does not apply because the condition is incomplete. You 
>>somehow dropped this from the condition:
>>
>>                   vcpu_is_preempted(task_cpu(owner)))
>>
>>Please make always sure that your patches apply against Linus tree 
>>before sending them out.
>
> This commit c0bed69daf4b ("locking: Make owner_on_cpu() into 
> <linux/sched.h>") make the following modifications in latest linux-next.
>
> +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));
> +}
> +
>
>Fine, but then please tell against which tree/branch the patch is.

linux-next/master, linux-next/akpm, linux-next/akpm-base.

Thanks,

Zqiang

>
>Thanks,
>
>        tglx

[tip: locking/core] locking/rtmutex: Fix incorrect condition in rtmutex_spin_on_owner()

[tip-bot2 for Zqiang]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     8f556a326c93213927e683fc32bbf5be1b62540a
Gitweb:        https://git.kernel.org/tip/8f556a326c93213927e683fc32bbf5be1b62540a
Author:        Zqiang <qiang1.zhang@intel.com>
AuthorDate:    Fri, 17 Dec 2021 15:42:07 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 18 Dec 2021 10:55:51 +01:00

locking/rtmutex: Fix incorrect condition in rtmutex_spin_on_owner()

Optimistic spinning needs to be terminated when the spinning waiter is not
longer the top waiter on the lock, but the condition is negated. It
terminates if the waiter is the top waiter, which is defeating the whole
purpose.

Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter lockless")
Signed-off-by: Zqiang <qiang1.zhang@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20211217074207.77425-1-qiang1.zhang@intel.com
---
 kernel/locking/rtmutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 0c6a48d..1f25a4d 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1380,7 +1380,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 		 *  - the VCPU on which owner runs is preempted
 		 */
 		if (!owner->on_cpu || need_resched() ||
-		    rt_mutex_waiter_is_top_waiter(lock, waiter) ||
+		    !rt_mutex_waiter_is_top_waiter(lock, waiter) ||
 		    vcpu_is_preempted(task_cpu(owner))) {
 			res = false;
 			break;

Re: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Waiman Long]

On 12/17/21 02:42, Zqiang wrote:
> When the lock owner is on CPU and not need resched, the current waiter
> need to be checked, if it not longer top the waiter, stop spinning.

Incorrect grammar, should be "if it is no longer the top waiter". There 
is a similar typo in the existing code comment too.

You can modify the subject line to [PATCH-tip ...] to indicate that it 
is supposed to be apply on top of the tip tree. Other than that, the 
patch looks good.

Cheers,
Longman

>
> Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter lockless")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
>   v1->v2:
>   Modify description information.
>
>   kernel/locking/rtmutex.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 0c1f2e3f019a..8555c4efe97c 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1383,7 +1383,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
>   		 *  - the VCPU on which owner runs is preempted
>   		 */
>   		if (!owner_on_cpu(owner) || need_resched() ||
> -		    rt_mutex_waiter_is_top_waiter(lock, waiter)) {
> +		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
>   			res = false;
>   			break;
>   		}

RE: [PATCH v2] locking/rtmutex: Fix incorrect spinning condition

[Zhang, Qiang1]

> When the lock owner is on CPU and not need resched, the current waiter 
> need to be checked, if it not longer top the waiter, stop spinning.
>
>Incorrect grammar, should be "if it is no longer the top waiter". There is a similar typo in the existing code comment too.
>
>You can modify the subject line to [PATCH-tip ...] to indicate that it is supposed to be apply on top of the tip tree. Other than that, the patch looks good.

Thanks, Longman. I will  modify it and resend.

Thanks,

Zqiang
>
>Cheers,
>Longman

>
> Fixes: c3123c431447 ("locking/rtmutex: Dont dereference waiter 
> lockless")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
>   v1->v2:
>   Modify description information.
>
>   kernel/locking/rtmutex.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c index 
> 0c1f2e3f019a..8555c4efe97c 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1383,7 +1383,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
>   		 *  - the VCPU on which owner runs is preempted
>   		 */
>   		if (!owner_on_cpu(owner) || need_resched() ||
> -		    rt_mutex_waiter_is_top_waiter(lock, waiter)) {
> +		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
>   			res = false;
>   			break;
>   		}