Messages in this thread Patch in this message |  | | From | "Zhang, Qiang" <> | Subject | 回复: [PATCH v2] locking/mutex: clear MUTEX F LAGS if wait list is empty due to signal | Date | Mon, 17 May 2021 03:32:36 +0000 |
| |
________________________________________ 发件人: 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) >
|  |