From: Zqiang <[email protected]>
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 <[email protected]>
---
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
On 5/16/21 12:53 AM, [email protected] wrote:
> From: Zqiang <[email protected]>
>
> 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
<[email protected]>" 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 <[email protected]>
> ---
> 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)
>
________________________________________
??????: Waiman Long <[email protected]>
????ʱ??: 2021??5??17?? 1:22
?ռ???: Zhang, Qiang; [email protected]; [email protected]; [email protected]; [email protected]
????: [email protected]
????: 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, [email protected] wrote:
> From: Zqiang <[email protected]>
>
> 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
><[email protected]>" 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 <[email protected]>
> ---
> 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)
>