[PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal

[PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal

[qiang.zhang]

From: Zqiang <qiang.zhang@windriver.com>

When call mutex_lock_interruptible(), if after queue waiter to
lock->wait_list, exit cycle interrupted by signal without obtaining
lock, the waiter be del from lock->wait_list, if the lock->wait_list
is empty, and not clear MUTEX_FLAGS, when the lock is acquired again
, because the lock flags exist, the trylock_fast will be failed,
and enter slow path acqurie lock, so clear MUTEX_FLAGS when call
mutex_lock_interruptible() interrupted by a signal and the
lock->wait_list is empty,  in this way, when the lock is acquired
again, it will acquire succeed in the fast path.

Signed-off-by: Zqiang <qiang.zhang@windriver.com>
---
 v1->v2:
 Make commit info clearer and modify the code again.

 kernel/locking/mutex-debug.c |  4 ++--
 kernel/locking/mutex-debug.h |  2 +-
 kernel/locking/mutex.c       | 16 +++++++++++-----
 kernel/locking/mutex.h       |  4 +---
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index a7276aaf2abc..db9301591e3f 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -57,7 +57,7 @@ void debug_mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
 	task->blocked_on = waiter;
 }
 
-void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
+void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
 			 struct task_struct *task)
 {
 	DEBUG_LOCKS_WARN_ON(list_empty(&waiter->list));
@@ -65,7 +65,7 @@ void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
 	DEBUG_LOCKS_WARN_ON(task->blocked_on != waiter);
 	task->blocked_on = NULL;
 
-	list_del_init(&waiter->list);
+	INIT_LIST_HEAD(&waiter->list);
 	waiter->task = NULL;
 }
 
diff --git a/kernel/locking/mutex-debug.h b/kernel/locking/mutex-debug.h
index 1edd3f45a4ec..53e631e1d76d 100644
--- a/kernel/locking/mutex-debug.h
+++ b/kernel/locking/mutex-debug.h
@@ -22,7 +22,7 @@ extern void debug_mutex_free_waiter(struct mutex_waiter *waiter);
 extern void debug_mutex_add_waiter(struct mutex *lock,
 				   struct mutex_waiter *waiter,
 				   struct task_struct *task);
-extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
+extern void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
 				struct task_struct *task);
 extern void debug_mutex_unlock(struct mutex *lock);
 extern void debug_mutex_init(struct mutex *lock, const char *name,
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index cb6b112ce155..4815162d04b1 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -205,6 +205,15 @@ __mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
 		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
 }
 
+static void
+__mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
+{
+	__list_del(waiter->list.prev, waiter->list.next);
+	debug_mutex_remove_waiter(lock, waiter, current);
+	if (likely(list_empty(&lock->wait_list)))
+		__mutex_clear_flag(lock, MUTEX_FLAGS);
+}
+
 /*
  * Give up ownership to a specific task, when @task = NULL, this is equivalent
  * to a regular unlock. Sets PICKUP on a handoff, clears HANDOFF, preserves
@@ -1061,10 +1070,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 			__ww_mutex_check_waiters(lock, ww_ctx);
 	}
 
-	mutex_remove_waiter(lock, &waiter, current);
-	if (likely(list_empty(&lock->wait_list)))
-		__mutex_clear_flag(lock, MUTEX_FLAGS);
-
+	__mutex_remove_waiter(lock, &waiter);
 	debug_mutex_free_waiter(&waiter);
 
 skip_wait:
@@ -1080,7 +1086,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 
 err:
 	__set_current_state(TASK_RUNNING);
-	mutex_remove_waiter(lock, &waiter, current);
+	__mutex_remove_waiter(lock, &waiter);
 err_early_kill:
 	spin_unlock(&lock->wait_lock);
 	debug_mutex_free_waiter(&waiter);
diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
index 1c2287d3fa71..f0c710b1d192 100644
--- a/kernel/locking/mutex.h
+++ b/kernel/locking/mutex.h
@@ -10,12 +10,10 @@
  * !CONFIG_DEBUG_MUTEXES case. Most of them are NOPs:
  */
 
-#define mutex_remove_waiter(lock, waiter, task) \
-		__list_del((waiter)->list.prev, (waiter)->list.next)
-
 #define debug_mutex_wake_waiter(lock, waiter)		do { } while (0)
 #define debug_mutex_free_waiter(waiter)			do { } while (0)
 #define debug_mutex_add_waiter(lock, waiter, ti)	do { } while (0)
+#define debug_mutex_remove_waiter(lock, waiter, ti)     do { } while (0)
 #define debug_mutex_unlock(lock)			do { } while (0)
 #define debug_mutex_init(lock, name, key)		do { } while (0)
 
-- 
2.25.1

Re: [PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal

[Waiman Long]

On 5/16/21 12:53 AM, qiang.zhang@windriver.com wrote:
> From: Zqiang <qiang.zhang@windriver.com>
>
> When call mutex_lock_interruptible(), if after queue waiter to
> lock->wait_list, exit cycle interrupted by signal without obtaining
> lock, the waiter be del from lock->wait_list, if the lock->wait_list
> is empty, and not clear MUTEX_FLAGS, when the lock is acquired again
> , because the lock flags exist, the trylock_fast will be failed,
> and enter slow path acqurie lock, so clear MUTEX_FLAGS when call
> mutex_lock_interruptible() interrupted by a signal and the
> lock->wait_list is empty,  in this way, when the lock is acquired
> again, it will acquire succeed in the fast path.

Well, you have managed to put all these information into one English 
sentence:-)

Anyway, this is not proper English and you need to break it down into 
several sentences.

After looking at the code again, this bug is not a correctness issue. It 
is mainly a performance issue. If the mutex isn't contended enough to 
have a waiter put into the wait queue again, the setting of the WAITER 
bit will force mutex locker to go into the slowpath to acquire the lock 
every time.

BTW, you should have put "Suggested-by: Peter Zijlstra 
<peterz@infradead.org>" before your signed-off line as an attribution to 
him as he had suggested the change in this patch.

Cheers,
Longman

>
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> ---
>   v1->v2:
>   Make commit info clearer and modify the code again.
>
>   kernel/locking/mutex-debug.c |  4 ++--
>   kernel/locking/mutex-debug.h |  2 +-
>   kernel/locking/mutex.c       | 16 +++++++++++-----
>   kernel/locking/mutex.h       |  4 +---
>   4 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index a7276aaf2abc..db9301591e3f 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -57,7 +57,7 @@ void debug_mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   	task->blocked_on = waiter;
>   }
>   
> -void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   			 struct task_struct *task)
>   {
>   	DEBUG_LOCKS_WARN_ON(list_empty(&waiter->list));
> @@ -65,7 +65,7 @@ void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   	DEBUG_LOCKS_WARN_ON(task->blocked_on != waiter);
>   	task->blocked_on = NULL;
>   
> -	list_del_init(&waiter->list);
> +	INIT_LIST_HEAD(&waiter->list);
>   	waiter->task = NULL;
>   }
>   
> diff --git a/kernel/locking/mutex-debug.h b/kernel/locking/mutex-debug.h
> index 1edd3f45a4ec..53e631e1d76d 100644
> --- a/kernel/locking/mutex-debug.h
> +++ b/kernel/locking/mutex-debug.h
> @@ -22,7 +22,7 @@ extern void debug_mutex_free_waiter(struct mutex_waiter *waiter);
>   extern void debug_mutex_add_waiter(struct mutex *lock,
>   				   struct mutex_waiter *waiter,
>   				   struct task_struct *task);
> -extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +extern void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   				struct task_struct *task);
>   extern void debug_mutex_unlock(struct mutex *lock);
>   extern void debug_mutex_init(struct mutex *lock, const char *name,
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index cb6b112ce155..4815162d04b1 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -205,6 +205,15 @@ __mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
>   }
>   
> +static void
> +__mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> +{
> +	__list_del(waiter->list.prev, waiter->list.next);
> +	debug_mutex_remove_waiter(lock, waiter, current);
> +	if (likely(list_empty(&lock->wait_list)))
> +		__mutex_clear_flag(lock, MUTEX_FLAGS);
> +}
> +
>   /*
>    * Give up ownership to a specific task, when @task = NULL, this is equivalent
>    * to a regular unlock. Sets PICKUP on a handoff, clears HANDOFF, preserves
> @@ -1061,10 +1070,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>   			__ww_mutex_check_waiters(lock, ww_ctx);
>   	}
>   
> -	mutex_remove_waiter(lock, &waiter, current);
> -	if (likely(list_empty(&lock->wait_list)))
> -		__mutex_clear_flag(lock, MUTEX_FLAGS);
> -
> +	__mutex_remove_waiter(lock, &waiter);
>   	debug_mutex_free_waiter(&waiter);
>   
>   skip_wait:
> @@ -1080,7 +1086,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>   
>   err:
>   	__set_current_state(TASK_RUNNING);
> -	mutex_remove_waiter(lock, &waiter, current);
> +	__mutex_remove_waiter(lock, &waiter);
>   err_early_kill:
>   	spin_unlock(&lock->wait_lock);
>   	debug_mutex_free_waiter(&waiter);
> diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
> index 1c2287d3fa71..f0c710b1d192 100644
> --- a/kernel/locking/mutex.h
> +++ b/kernel/locking/mutex.h
> @@ -10,12 +10,10 @@
>    * !CONFIG_DEBUG_MUTEXES case. Most of them are NOPs:
>    */
>   
> -#define mutex_remove_waiter(lock, waiter, task) \
> -		__list_del((waiter)->list.prev, (waiter)->list.next)
> -
>   #define debug_mutex_wake_waiter(lock, waiter)		do { } while (0)
>   #define debug_mutex_free_waiter(waiter)			do { } while (0)
>   #define debug_mutex_add_waiter(lock, waiter, ti)	do { } while (0)
> +#define debug_mutex_remove_waiter(lock, waiter, ti)     do { } while (0)
>   #define debug_mutex_unlock(lock)			do { } while (0)
>   #define debug_mutex_init(lock, name, key)		do { } while (0)
>   

回复: [PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal

[Zhang, Qiang]

________________________________________
发件人: Waiman Long <llong@redhat.com>
发送时间: 2021年5月17日 1:22
收件人: Zhang, Qiang; peterz@infradead.org; mingo@redhat.com; will@kernel.org; boqun.feng@gmail.com
抄送: linux-kernel@vger.kernel.org
主题: Re: [PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal

[Please note: This e-mail is from an EXTERNAL e-mail address]

On 5/16/21 12:53 AM, qiang.zhang@windriver.com wrote:
> From: Zqiang <qiang.zhang@windriver.com>
>
> When call mutex_lock_interruptible(), if after queue waiter to
> lock->wait_list, exit cycle interrupted by signal without obtaining
> lock, the waiter be del from lock->wait_list, if the lock->wait_list
> is empty, and not clear MUTEX_FLAGS, when the lock is acquired again
> , because the lock flags exist, the trylock_fast will be failed,
> and enter slow path acqurie lock, so clear MUTEX_FLAGS when call
> mutex_lock_interruptible() interrupted by a signal and the
> lock->wait_list is empty,  in this way, when the lock is acquired
> again, it will acquire succeed in the fast path.

>Well, you have managed to put all these information into one English
>sentence:-)
>
>Anyway, this is not proper English and you need to break it >down into
>several sentences.
>
>After looking at the code again, this bug is not a correctness >issue. It
>is mainly a performance issue. If the mutex isn't contended >enough to
>have a waiter put into the wait queue again, the setting of >the WAITER
>bit will force mutex locker to go into the slowpath to acquire >the lock
>every time.
>
>BTW, you should have put "Suggested-by: Peter Zijlstra
><peterz@infradead.org>" before your signed-off line as an >attribution to
>him as he had suggested the change in this patch.
>
 
Thank for  your suggest
 I will resend v3 patch.

Cheers
Qiang
>Cheers,
>Longman

>
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> ---
>   v1->v2:
>   Make commit info clearer and modify the code again.
>
>   kernel/locking/mutex-debug.c |  4 ++--
>   kernel/locking/mutex-debug.h |  2 +-
>   kernel/locking/mutex.c       | 16 +++++++++++-----
>   kernel/locking/mutex.h       |  4 +---
>   4 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index a7276aaf2abc..db9301591e3f 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -57,7 +57,7 @@ void debug_mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>       task->blocked_on = waiter;
>   }
>
> -void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>                        struct task_struct *task)
>   {
>       DEBUG_LOCKS_WARN_ON(list_empty(&waiter->list));
> @@ -65,7 +65,7 @@ void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>       DEBUG_LOCKS_WARN_ON(task->blocked_on != waiter);
>       task->blocked_on = NULL;
>
> -     list_del_init(&waiter->list);
> +     INIT_LIST_HEAD(&waiter->list);
>       waiter->task = NULL;
>   }
>
> diff --git a/kernel/locking/mutex-debug.h b/kernel/locking/mutex-debug.h
> index 1edd3f45a4ec..53e631e1d76d 100644
> --- a/kernel/locking/mutex-debug.h
> +++ b/kernel/locking/mutex-debug.h
> @@ -22,7 +22,7 @@ extern void debug_mutex_free_waiter(struct mutex_waiter *waiter);
>   extern void debug_mutex_add_waiter(struct mutex *lock,
>                                  struct mutex_waiter *waiter,
>                                  struct task_struct *task);
> -extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +extern void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>                               struct task_struct *task);
>   extern void debug_mutex_unlock(struct mutex *lock);
>   extern void debug_mutex_init(struct mutex *lock, const char *name,
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index cb6b112ce155..4815162d04b1 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -205,6 +205,15 @@ __mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>               __mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
>   }
>
> +static void
> +__mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> +{
> +     __list_del(waiter->list.prev, waiter->list.next);
> +     debug_mutex_remove_waiter(lock, waiter, current);
> +     if (likely(list_empty(&lock->wait_list)))
> +             __mutex_clear_flag(lock, MUTEX_FLAGS);
> +}
> +
>   /*
>    * Give up ownership to a specific task, when @task = NULL, this is equivalent
>    * to a regular unlock. Sets PICKUP on a handoff, clears HANDOFF, preserves
> @@ -1061,10 +1070,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>                       __ww_mutex_check_waiters(lock, ww_ctx);
>       }
>
> -     mutex_remove_waiter(lock, &waiter, current);
> -     if (likely(list_empty(&lock->wait_list)))
> -             __mutex_clear_flag(lock, MUTEX_FLAGS);
> -
> +     __mutex_remove_waiter(lock, &waiter);
>       debug_mutex_free_waiter(&waiter);
>
>   skip_wait:
> @@ -1080,7 +1086,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>
>   err:
>       __set_current_state(TASK_RUNNING);
> -     mutex_remove_waiter(lock, &waiter, current);
> +     __mutex_remove_waiter(lock, &waiter);
>   err_early_kill:
>       spin_unlock(&lock->wait_lock);
>       debug_mutex_free_waiter(&waiter);
> diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
> index 1c2287d3fa71..f0c710b1d192 100644
> --- a/kernel/locking/mutex.h
> +++ b/kernel/locking/mutex.h
> @@ -10,12 +10,10 @@
>    * !CONFIG_DEBUG_MUTEXES case. Most of them are NOPs:
>    */
>
> -#define mutex_remove_waiter(lock, waiter, task) \
> -             __list_del((waiter)->list.prev, (waiter)->list.next)
> -
>   #define debug_mutex_wake_waiter(lock, waiter)               do { } while (0)
>   #define debug_mutex_free_waiter(waiter)                     do { } while (0)
>   #define debug_mutex_add_waiter(lock, waiter, ti)    do { } while (0)
> +#define debug_mutex_remove_waiter(lock, waiter, ti)     do { } while (0)
>   #define debug_mutex_unlock(lock)                    do { } while (0)
>   #define debug_mutex_init(lock, name, key)           do { } while (0)
>