Messages in this thread | | | Date | Fri, 15 Jul 2016 15:47:36 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2 2/5] locking/pvqspinlock: Fix missed PV wakeup problem |
| |
On 07/15/2016 04:47 AM, Peter Zijlstra wrote: > So the reason I never get around to this is because the patch stinks. > > It simply doesn't make sense... Remember, the harder you make a reviewer > work the less likely the review will be done. > > Present things in clear concise language and draw a picture. > > On Tue, May 31, 2016 at 12:53:48PM -0400, Waiman Long wrote: >> Currently, calling pv_hash() and setting _Q_SLOW_VAL is only >> done once for any pv_node. It is either in pv_kick_node() or in >> pv_wait_head_or_lock(). > So far so good.... > >> Because of lock stealing, a pv_kick'ed node is >> not guaranteed to get the lock before the spinning threshold expires >> and has to call pv_wait() again. As a result, the new lock holder >> won't see _Q_SLOW_VAL and so won't wake up the sleeping vCPU. > *brain melts* what!? pv_kick'ed node reads like pv_kick_node() and that > doesn't make any kind of sense.
Sorry for the confusing. I will clean up the submit log to discuss what I actually mean.
> 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); >
Yes, that is the scenario that I have in mind.
> 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. > > Is this indeed the 3 CPU scenario you tried to describe in a scant 4 > lines of text, or is there more to it?
You are right. The vCPU won't actually going to wait. It will get out and spin again. I will correct the patch title. However, it is still not good as it is not doing what it is suppose to do.
Cheers, Longman
| |