Re: [PATCH] locking/pvqspinlock: restore/set vcpu_hashed state after failing adaptive locking spinning

[xinhui]

On 2016å07æ14æ 15:53, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>
> When the lock holder vCPU is racing with the queue head:
>
>     CPU 0 (lock holder)    CPU1 (queue head)
>     ===================    =================
>     spin_lock();           spin_lock();
>      pv_kick_node();        pv_wait_head_or_lock();
>                              if (!lp) {
>                               lp = pv_hash(lock, pn);
> 							 xchg(&l->locked, _Q_SLOW_VAL);
> 							}
> 						    WRITE_ONCE(pn->state, vcpu_halted);
>       cmpxchg(&pn->state,
>        vcpu_halted, vcpu_hashed);
>       WRITE_ONCE(l->locked, _Q_SLOW_VAL);
>       (void)pv_hash(lock, pn);
>
> In this case, lock holder inserts the pv_node of queue head into the
> hash table and set _Q_SLOW_VAL unnecessary. This patch avoids it by
> restoring/setting vcpu_halted state after failing adaptive locking
> spinning.

good catching. this is hash entry leak.

After apply Waiman's pathset "locking/pvqspinlock: Fix missed PV wakeup & support PPC", this is not the big problem.
But we can still save another cmpxchg.

So
Reviewed-by: Pan Xinhui <xinhui.pan@xxxxxxxxxxxxxxxxxx>

> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Waiman Long <Waiman.Long@xxxxxxx>
> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> Signed-off-by: Wanpeng Li <wanpeng.li@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 21ede57..ac7d20b 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -450,7 +450,7 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct mcs_spinlock *node)
>   				goto gotlock;
>   			}
>   		}
> -		WRITE_ONCE(pn->state, vcpu_halted);
> +		WRITE_ONCE(pn->state, vcpu_hashed);
>   		qstat_inc(qstat_pv_wait_head, true);
>   		qstat_inc(qstat_pv_wait_again, waitcnt);
>   		pv_wait(&l->locked, _Q_SLOW_VAL);