Re: [PATCH v2 2/5] locking/pvqspinlock: Fix missed PV wakeup problem

[Waiman Long <waiman.long@hpe.com>]

On 07/15/2016 06:07 AM, Peter Zijlstra wrote:
> On Fri, Jul 15, 2016 at 05:39:46PM +0800, Pan Xinhui wrote:
>>> I'm thinking you're trying to say this:
>>>
>>>
>>> CPU0			CPU1			CPU2
>>>
>>> __pv_queued_spin_unlock_slowpath()
>>>   ...
>>>   smp_store_release(&l->locked, 0);
>>> 			__pv_queued_spin_lock_slowpath()
>>> 			  ...
>>> 			  pv_queued_spin_steal_lock()
>>> 			    cmpxchg(&l->locked, 0, _Q_LOCKED_VAL) == 0
>>>
>>>
>>> 						pv_wait_head_or_lock()
>>>
>>>   pv_kick(node->cpu);  ---------------------->	pv_wait(&l->locked, _Q_SLOW_VAL);
>>>
>>> 			__pv_queued_spin_unlock()
>>> 			  cmpxchg(&l->locked, _Q_LOCKED_VAL, 0) == _Q_LOCKED_VAL
>>>
>>> 						  for () {
>>> 						    trylock_clear_pending();
>>> 						    cpu_relax();
>>> 						  }
>>>
>>> 						  pv_wait(&l->locked, _Q_SLOW_VAL);
>>>
>>>
>>> Which is indeed 'bad', but not fatal, note that the later pv_wait() will
>>> not in fact go wait, since l->locked will _not_ be _Q_SLOW_VAL.
>> the problem is that "this later pv_wait will do nothing as l->locked
>> is not _Q_SLOW_VAL", So it is not paravirt friendly then. we will go
>> into the trylock loop again and again until the lock is unlocked.
> Agreed, which is 'bad'. But the patch spoke about a missing wakeup,
> which is worse, as that would completely inhibit progress.

Sorry, it is my mistake. There is no missing pv_wait().

>> So if we are kicked by the unlock_slowpath, and the lock is stealed by
>> someone else,  we need hash its node again and set l->locked to
>> _Q_SLOW_VAL, then enter pv_wait.
> Right, let me go think about this a bit.

Yes, the purpose of this patch is to do exactly that. Let's the queue 
head vCPU sleeps until the lock holder release the lock and wake the 
queue head vCPU up.

>
>> but I am worried about lock stealing. could the node in the queue
>> starve for a long time? I notice the latency of pv_wait on an
>> over-commited guest can be bigger than 300us. I have not seen such
>> starving case, but I think it is possible to happen.
> I share that worry, which is why we limit the steal attempt to one.
> But yes, theoretically its possible to starve things AFAICT.
>
> We've not come up with sensible way to completely avoid starvation.

If you guys are worrying about lock constantly getting stolen between 
pv_kick() of queue head vCPU and it is ready to take the lock, we can 
keep the pending bit set across pv_wait() if it is the 2nd or later time 
that pv_wait() is called. That will ensure that no lock stealing can 
happen and cap the maximum wait time to about 2x (spin + pv_wait). I 
will add that patch to my patch series.

Cheers,
Longman