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

[Waiman Long]

On 5/18/21 6:49 AM, Peter Zijlstra wrote:
> On Mon, May 17, 2021 at 11:40:05AM +0800, qiang.zhang@xxxxxxxxxxxxx wrote:
> > From: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
> >
> > When a interruptible mutex locker is interrupted by a signal
> > without acquiring this lock and removed from the wait queue.
> > 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, so if the wait queue is empty,
> > the WAITER bit need to be clear.
> I'm still interestd in knowing how you found this. Did you have an
> actual problem, or were you just reading the code?
>
> AFAICT, this needs:
>
> Fixes: 040a0a371005 ("mutex: Add support for wound/wait style locks")
>
> > Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
> Thanks!
>
> Updated patch below.
>
> ---
> Subject: locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal
> From: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
> Date: Mon, 17 May 2021 11:40:05 +0800
>
> From: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
>
> When a interruptible mutex locker is interrupted by a signal
> without acquiring this lock and removed from the wait queue.
> 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, so if the wait queue is empty,
> the WAITER bit need to be clear.
>
> Fixes: 040a0a371005 ("mutex: Add support for wound/wait style locks")
> Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Link: https://lkml.kernel.org/r/20210517034005.30828-1-qiang.zhang@xxxxxxxxxxxxx
> ---
>   kernel/locking/mutex-debug.c |    4 ++--
>   kernel/locking/mutex-debug.h |    2 +-
>   kernel/locking/mutex.c       |   18 +++++++++++++-----
>   kernel/locking/mutex.h       |    4 +---
>   4 files changed, 17 insertions(+), 11 deletions(-)
>
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -57,7 +57,7 @@ void debug_mutex_add_waiter(struct mutex
>   	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 *l
>   	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;
>   }
>   
> --- a/kernel/locking/mutex-debug.h
> +++ b/kernel/locking/mutex-debug.h
> @@ -22,7 +22,7 @@ extern void debug_mutex_free_waiter(stru
>   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,
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -194,7 +194,7 @@ static inline bool __mutex_waiter_is_fir
>    * Add @waiter to a given location in the lock wait_list and set the
>    * FLAG_WAITERS flag if it's the first waiter.
>    */
> -static void __sched
> +static void
>   __mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
>   		   struct list_head *list)
>   {
> @@ -205,6 +205,16 @@ __mutex_add_waiter(struct mutex *lock, s
>   		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
>   }
>   
> +static void
> +__mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> +{
> +	list_del(&waiter->list);
> +	if (likely(list_empty(&lock->wait_list)))
> +		__mutex_clear_flag(lock, MUTEX_FLAGS);
> +
> +	debug_mutex_remove_waiter(lock, waiter, current);
> +}
> +
>   /*
>    * 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,9 +1071,7 @@ __mutex_lock_common(struct mutex *lock,
>   			__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);
>   
> @@ -1080,7 +1088,7 @@ __mutex_lock_common(struct mutex *lock,
>   
>   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);
> --- 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)
>   
Reviewed-by: Waiman Long <longman@xxxxxxxxxx>