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: Reduce chance of setting HANDOFF bit on
 unlocked mutex
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        linux-kernel@vger.kernel.org,
        "Xu, Yanfei" <yanfei.xu@windriver.com>
References: <20210629201138.31507-1-longman@redhat.com>
 <YNxFkD8qzex9MvQp@hirez.programming.kicks-ass.net>
Message-ID: <ecc0cc97-23ca-5de3-2a12-ed50aa12548c@redhat.com>
Date:   Wed, 30 Jun 2021 09:50:11 -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: <YNxFkD8qzex9MvQp@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 6/30/21 6:21 AM, Peter Zijlstra wrote:
> On Tue, Jun 29, 2021 at 04:11:38PM -0400, Waiman Long wrote:
>
>> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
>> index d2df5e68b503..472ab21b5b8e 100644
>> --- a/kernel/locking/mutex.c
>> +++ b/kernel/locking/mutex.c
>> @@ -118,9 +118,9 @@ static inline struct task_struct *__mutex_trylock_or_owner(struct mutex *lock)
>>   		}
>>   
>>   		/*
>> -		 * We set the HANDOFF bit, we must make sure it doesn't live
>> -		 * past the point where we acquire it. This would be possible
>> -		 * if we (accidentally) set the bit on an unlocked mutex.
>> +		 * Always clear the HANDOFF bit before acquiring the lock.
>> +		 * Note that if the bit is accidentally set on an unlocked
>> +		 * mutex, anyone can acquire it.
>>   		 */
>>   		flags &= ~MUTEX_FLAG_HANDOFF;
>>   
>> @@ -180,6 +180,11 @@ static inline void __mutex_set_flag(struct mutex *lock, unsigned long flag)
>>   	atomic_long_or(flag, &lock->owner);
>>   }
>>   
>> +static inline long __mutex_fetch_set_flag(struct mutex *lock, unsigned long flag)
>> +{
>> +	return atomic_long_fetch_or_relaxed(flag, &lock->owner);
>> +}
>> +
>>   static inline void __mutex_clear_flag(struct mutex *lock, unsigned long flag)
>>   {
> Hurmph, so we already have a cmpxchg loop in trylock, might as well make
> that do exactly what we want without holes on.
>
> How's something like the below? Boot tested, but please verify.
>
> ---
>   kernel/locking/mutex.c | 89 ++++++++++++++++++++++++++------------------------
>   1 file changed, 46 insertions(+), 43 deletions(-)

The code looks good to me. It is an even better approach to make sure 
that the HANDOFF will never be set on an unlocked mutex.

Reviewed-by: Waiman Long <longman@redhat.com>