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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
-----Original Message-----
From: Thomas Gleixner <[email protected]>
Sent: 2021??12??18?? 4:53
To: Zhang, Qiang1 <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Zhang, Qiang1 <[email protected]>
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 <[email protected]>
> ---
> 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
On Sat, Dec 18 2021 at 07:24, Qiang1 Zhang wrote:
> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: 2021年12月18日 4:53
> To: Zhang, Qiang1 <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
> Cc: [email protected]; Zhang, Qiang1 <[email protected]>
> 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
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 <[email protected]>
AuthorDate: Fri, 17 Dec 2021 15:42:07 +08:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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;
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 <[email protected]>
> ---
> 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;
> }
>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
> 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 <[email protected]>
> ---
> 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;
> }