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] locking/mutex: fix the MUTEX_FLAG_HANDOFF bit is cleared
 unexpected
To:     Yanfei Xu <yanfei.xu@windriver.com>, peterz@infradead.org,
        mingo@redhat.com, boqun.feng@gmail.com
Cc:     linux-kernel@vger.kernel.org
References: <20210628155155.11623-1-yanfei.xu@windriver.com>
Message-ID: <e914d4fd-5afc-35f9-c068-7044ceda53dd@redhat.com>
Date:   Mon, 28 Jun 2021 13:57:40 -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: <20210628155155.11623-1-yanfei.xu@windriver.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 6/28/21 11:51 AM, Yanfei Xu wrote:
> When the mutex unlock path is excuted with WAITERS bit and without
> HANDOFF bit set, it will wake up the first task in wait_list. If
> there are some tasks not in wait_list are stealing lock, it is very
> likely successfully due to the task field of lock is NULL and flags
> field is non-NULL. Then the HANDOFF bit will be cleared. But if the
> HANDOFF bit was just set by the waked task in wait_list, this clearing
> is unexcepted.

I think you mean "unexpected". Right? Anyway, all the setting and 
clearing of the HANDOFF bit are atomic. There shouldn't be any 
unexpected clearing.

> __mutex_lock_common                   __mutex_lock_common
>    __mutex_trylock                       schedule_preempt_disabled
>      /*steal lock successfully*/         __mutex_set_flag(lock,MUTEX_FLAG_HANDOFF)
>      __mutex_trylock_or_owner
>        if (task==NULL)
>        flags &= ~MUTEX_FLAG_HANDOFF
>        atomic_long_cmpxchg_acquire
>                                          __mutex_trylock  //failed
>                                          mutex_optimistic_spin  //failed
>                                          schedule_preempt_disabled  //sleep without HANDOFF bit
>
> So the HANDOFF bit should be set as late as possible, here we defer
> it util the task is going to be scheduled.
> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>
> ---
>
> Hi maintainers,
>
> I am not very sure if I missed or misunderstanded something, please help
> to review. Many thanks!
>
>   kernel/locking/mutex.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 013e1b08a1bf..e57d920e96bf 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -1033,17 +1033,17 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>   		}
>   
>   		spin_unlock(&lock->wait_lock);
> +
> +		if (first)
> +			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
>   		schedule_preempt_disabled();
>   
>   		/*
>   		 * ww_mutex needs to always recheck its position since its waiter
>   		 * list is not FIFO ordered.
>   		 */
> -		if (ww_ctx || !first) {
> +		if (ww_ctx || !first)
>   			first = __mutex_waiter_is_first(lock, &waiter);
> -			if (first)
> -				__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> -		}
>   
>   		set_current_state(state);
>   		/*

In general, I don't mind setting the HANDOFF bit later, but 
mutex_optimistic_spin() at the end of the loop should only be called 
after the HANDOFF bit is set. So the logic isn't quite right yet.

Cheers,
Longman