Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3e580bf2-2c76-739e-7636-4aa03f5628bd@redhat.com>
Date:   Mon, 8 May 2023 22:30:39 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 1/1] locking/qspinlock: Fix state-transition changes in
 comments
Content-Language: en-US
To:     "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>
Cc:     Boqun Feng <boqun.feng@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230506062934.69652-1-qiuxu.zhuo@intel.com>
 <10d3a11a-8591-e49d-e010-867a05078ff0@redhat.com>
 <IA1PR11MB617110DB70F92AFC0D40D26089769@IA1PR11MB6171.namprd11.prod.outlook.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <IA1PR11MB617110DB70F92AFC0D40D26089769@IA1PR11MB6171.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 5/8/23 22:03, Zhuo, Qiuxu wrote:
> Hi Wainman,
>
> Thanks for your review. Please see the comments below.
>
>> From: Waiman Long <longman@redhat.com>
>> Sent: Monday, May 8, 2023 11:29 PM
>> To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>; Peter Zijlstra
>> <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>; Will Deacon
>> <will@kernel.org>
>> Cc: Boqun Feng <boqun.feng@gmail.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/1] locking/qspinlock: Fix state-transition changes in
>> comments
>>
>>
>> On 5/6/23 02:29, Qiuxu Zhuo wrote:
>>> 1. There may be concurrent locker CPUs to set the qspinlock pending bit.
>>>
>>>      The first CPU (called pending CPU) of these CPUs sets the pending
>>>      bit to make the state transition (the qspinlock pending bit is set):
>>>
>>>            0,0,* -> 0,1,*
>>>
>>>      The rest of these CPUs are queued to the MCS queue to make the state
>>>      transition (the qspinlock tail is updated):
>>>
>>>            0,1,* -> *,1,*
>>>
>>>      The pending CPU waits until the locker owner goes away to make
>>>      the state transition (the qspinlock locked field is set to zero):
>>>
>>>            *,1,* -> *,1,0
>>>
>>>      The pending CPU takes the ownership and clears the pending bit
>>>      to make the state transition:
>>>
>>>            *,1,0 -> *,0,1
>>>
>>> 2. The header of the MCS queue takes the ownership and calls set_locked()
>>>      to make the state transition:
>>>
>>>            *,*,0 -> *,*,1
>> That is not true. The pending bit owner has priority over the MCS queue
>> head. So the pending bit must be 0 before the MCS queue head can take over
>> the lock. So
>>
>>       *,0,0 -> *,0,1
> Yes, the pending bit must be 0 before the header can take the lock.
> But as the statement "There may be concurrent locker CPUs to set the qspinlock pending bit " at
> the beginning. So just after the header takes over the lock, there is also a possible concurrent locker CPU
> to set the pending bit. That means at this time point here, the pending bit could be either 0 or 1.

OK, I am looking from the point of view of the CPU doing the transition. 
Otherwise, for an arch with very weak memory ordering, anything is 
possible. i.e. *,*,* -> *,*,* if we have to consider what all the CPUs 
in the system can see.


>
>>> Fix the state-transition changes above in the code comments accordingly.
>>>
>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>> ---
>>>    kernel/locking/qspinlock.c | 10 ++++++----
>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>>> index ebe6b8ec7cb3..efebbf19f887 100644
>>> --- a/kernel/locking/qspinlock.c
>>> +++ b/kernel/locking/qspinlock.c
>>> @@ -257,7 +257,7 @@ static __always_inline u32
>> queued_fetch_set_pending_acquire(struct qspinlock *lo
>>>     * set_locked - Set the lock bit and own the lock
>>>     * @lock: Pointer to queued spinlock structure
>>>     *
>>> - * *,*,0 -> *,0,1
>>> + * *,*,0 -> *,*,1
>> set_locked() can only be called when it is sure that the pending bit isn't set.
>>>     */
>>>    static __always_inline void set_locked(struct qspinlock *lock)
>>>    {
>>> @@ -348,7 +348,7 @@ void __lockfunc queued_spin_lock_slowpath(struct
>> qspinlock *lock, u32 val)
>>>    	/*
>>>    	 * trylock || pending
>>>    	 *
>>> -	 * 0,0,* -> 0,1,* -> 0,0,1 pending, trylock
>>> +	 * 0,0,* -> 0,1,* -> ... -> *,0,1 pending, trylock
>> By the time trylock is done, there may be entries in the queue. However, I
>> doubt it helps by adding "..." in between possible multiple transitions.
>>
> The added "..." means there could be entries in the queue before trylock done.
> This is just for making the state transitions more complete ;-).
> If you think it doesn't help, I can remove it in the next version.

The transition flow does not show whether there is entry in the queue or 
not. It just shows the state of the lock word.


>
>>>    	 */
>>>    	val = queued_fetch_set_pending_acquire(lock);
>>>
>>> @@ -358,6 +358,8 @@ void __lockfunc queued_spin_lock_slowpath(struct
>> qspinlock *lock, u32 val)
>>>    	 * Undo and queue; our setting of PENDING might have made the
>>>    	 * n,0,0 -> 0,0,0 transition fail and it will now be waiting
>>>    	 * on @next to become !NULL.
>>> +	 *
>>> +	 * 0,1,* -> *,1,*
>> There is already a "n,0,0 -> 0,0,0" above, adding a new one may just
>> confuse people.
>>>    	 */
>>>    	if (unlikely(val & ~_Q_LOCKED_MASK)) {
>>>
>>> @@ -371,7 +373,7 @@ void __lockfunc queued_spin_lock_slowpath(struct
>> qspinlock *lock, u32 val)
>>>    	/*
>>>    	 * We're pending, wait for the owner to go away.
>>>    	 *
>>> -	 * 0,1,1 -> *,1,0
>>> +	 * *,1,* -> *,1,0
>> This refers to the wait loop. We don't need to wait if the owner has gone.
> But just before we wait for the locked field, the locked field could be
> either 0 (the locker can release the lock at any time) or 1.

Again, I take the viewpoint of the CPU doing the wait. It will only wait 
if it observes that the lock isn't free.


>
>>>    	 *
>>>    	 * this wait loop must be a load-acquire such that we match the
>>>    	 * store-release that clears the locked bit and create lock
>>> @@ -385,7 +387,7 @@ void __lockfunc queued_spin_lock_slowpath(struct
>> qspinlock *lock, u32 val)
>>>    	/*
>>>    	 * take ownership and clear the pending bit.
>>>    	 *
>>> -	 * 0,1,0 -> 0,0,1
>>> +	 * *,1,0 -> *,0,1
>> That is the part that we can make the change in the transition diagram
>> as noted.
> Sorry, I'm not clear about your request.
> Did you mean just make the change "*,1,0 -> *,0,1" above in the transition diagram or
> all the changes above in the transition diagram?

What I meant is that this particular change is correct AFAICS. Sorry for 
the confusion.

Cheers,
Longman