回复: [PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal
From: Zhang, Qiang
Date: Sun May 16 2021 - 23:33:26 EST
________________________________________
发件人: Waiman Long <llong@xxxxxxxxxx>
发送时间: 2021年5月17日 1:22
收件人: Zhang, Qiang; peterz@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; will@xxxxxxxxxx; boqun.feng@xxxxxxxxx
抄送: linux-kernel@xxxxxxxxxxxxxxx
主题: 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@xxxxxxxxxxxxx wrote:
> From: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>" 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@xxxxxxxxxxxxx>
> ---
> 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)
>