Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Waiman Long <llong@redhat.com>
Subject: Re: [PATCH v2] locking/mutex: clear MUTEX_FLAGS if wait_list is empty
 due to signal
To:     qiang.zhang@windriver.com, peterz@infradead.org, mingo@redhat.com,
        will@kernel.org, boqun.feng@gmail.com
Cc:     linux-kernel@vger.kernel.org
References: <20210516045354.348117-1-qiang.zhang@windriver.com>
Message-ID: <3ff1f3d3-19db-98a6-756d-48161c45acf9@redhat.com>
Date:   Sun, 16 May 2021 13:22:28 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <20210516045354.348117-1-qiang.zhang@windriver.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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)
>