Re: [PATCH 1/1] locking/qspinlock: Fix state-transition changes in comments
From: Waiman Long
Date: Mon May 08 2023 - 22:31:45 EST
On 5/8/23 22:03, Zhuo, Qiuxu wrote:
Hi Wainman,
Thanks for your review. Please see the comments below.
From: Waiman Long <longman@xxxxxxxxxx>
Sent: Monday, May 8, 2023 11:29 PM
To: Zhuo, Qiuxu <qiuxu.zhuo@xxxxxxxxx>; Peter Zijlstra
<peterz@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; Will Deacon
<will@xxxxxxxxxx>
Cc: Boqun Feng <boqun.feng@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
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@xxxxxxxxx>
---
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