Re: [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()

From: Waiman Long
Date: Mon Jul 22 2024 - 11:13:19 EST



On 7/22/24 02:54, maobibo wrote:
Uros,

Sorry for late reply because of weekend time. This modification works well for me.

In later I want to define pv_node::state as int type on LoongArch because there is only int32/int64 cmpxchg is better supported on the system, however that is another issue.

I thought about changing pv_node::state to int, but that requires changing the pv_wait() call to use int too. That doesn't work because pv_wait() is also used to monitor the state of the lock byte of the qspinlock. In essence, we can't use pvqspinlock if 8-bit cmpxchg isn't supported.

Cheers,
Longman


Tested-by:Bibo Mao <maobibo@xxxxxxxxxxx>

On 2024/7/22 上午12:45, Uros Bizjak wrote:
"enum vcpu_state" is not compatible with "u8" type for all targets,
resulting in:

error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *'

for LoongArch. Correct the type of "old" variable to "u8".

Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Will Deacon <will@xxxxxxxxxx>
Cc: Waiman Long <longman@xxxxxxxxxx>
Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h")
Reported-by: Bibo Mao <maobibo@xxxxxxxxxxx>
Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
Closes: https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@xxxxxxxxxxx/
---
  kernel/locking/qspinlock_paravirt.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
index f5a36e67b593..ac2e22502741 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
  static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
  {
      struct pv_node *pn = (struct pv_node *)node;
-    enum vcpu_state old = vcpu_halted;
+    u8 old = vcpu_halted;
      /*
       * If the vCPU is indeed halted, advance its state to match that of
       * pv_wait_node(). If OTOH this fails, the vCPU was running and will