Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576270 > unrolled thread

Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Boqun Feng <boqun.feng@gmail.com> - 2017-02-08 05:20 +0100
    Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Boqun Feng <boqun.feng@gmail.com> - 2017-02-08 07:10 +0100
      Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2017-02-08 07:50 +0100
      Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2017-02-08 07:50 +0100
      Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2017-02-08 08:10 +0100
        Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve  performance on some archs Boqun Feng <boqun.feng@gmail.com> - 2017-02-08 08:20 +0100

#1576270 — Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
> 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
> 
> > A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
> > relaxed versions to improve performance on architectures that use LL/SC.
> >
> > All the locking related cmpxchg's are replaced with the _acquire
> > variants:
> >  - pv_queued_spin_steal_lock()
> >  - trylock_clear_pending()
> >
> > The cmpxchg's related to hashing are replaced by either by the _release
> > or the _relaxed variants. See the inline comment for details.
> >
> > Signed-off-by: Waiman Long <longman@redhat.com>
> >
> >  v1->v2:
> >   - Add comments in changelog and code for the rationale of the change.
> >
> > ---
> >  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
> > -------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> >
> >
> > @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
> > struct mcs_spinlock *prev)
> >                  * If pv_kick_node() changed us to vcpu_hashed, retain that
> >                  * value so that pv_wait_head_or_lock() knows to not also
> > try
> >                  * to hash this lock.
> > +                *
> > +                * The smp_store_mb() and control dependency above will
> > ensure
> > +                * that state change won't happen before that.
> > Synchronizing
> > +                * with pv_kick_node() wrt hashing by this waiter or by the
> > +                * lock holder is done solely by the state variable. There
> > is
> > +                * no other ordering requirement.
> >                  */
> > -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
> > +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
> >
> >                 /*
> >                  * If the locked flag is still not set after wakeup, it is
> > a
> > @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
> > struct mcs_spinlock *node)
> >          * pv_wait_node(). If OTOH this fails, the vCPU was running and
> > will
> >          * observe its next->locked value and advance itself.
> >          *
> > -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
> > +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
> > pv_wait_node().
> > +        * A release barrier is used here to ensure that node->locked is
> > +        * always set before changing the state. See comment in
> > pv_wait_node().
> >          */
> > -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
> > +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
> > +                       != vcpu_halted)
> >                 return;
> >
> > hi, Waiman
> We can't use _release here, a full barrier is needed.
> 
> There is pv_kick_node vs pv_wait_head_or_lock
> 
> [w] l->locked = _Q_SLOW_VAL  //reordered here
> 
> if (READ_ONCE(pn->state) == vcpu_hashed) //False.
> 
>                    lp = (struct qspinlock **)1;
> 
> [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
> pn);
> pv_hash()                                                                if
> (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
> 

This analysis is correct, but..

> Then the same lock has hashed twice but only unhashed once. So at last as
> the hash table grows big, we hit RCU stall.
> 
> I hit RCU stall when I run netperf benchmark
> 

how will a big hash table hit RCU stall? Do you have the call trace for
your RCU stall?

Regards,
Boqun

> thanks
> xinhui
> 
> 
> > --
> > 1.8.3.1
> >
> >

[toc] | [next] | [standalone]

#1576288

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote:
> On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
> > 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
> > 
> > > A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
> > > relaxed versions to improve performance on architectures that use LL/SC.
> > >
> > > All the locking related cmpxchg's are replaced with the _acquire
> > > variants:
> > >  - pv_queued_spin_steal_lock()
> > >  - trylock_clear_pending()
> > >
> > > The cmpxchg's related to hashing are replaced by either by the _release
> > > or the _relaxed variants. See the inline comment for details.
> > >
> > > Signed-off-by: Waiman Long <longman@redhat.com>
> > >
> > >  v1->v2:
> > >   - Add comments in changelog and code for the rationale of the change.
> > >
> > > ---
> > >  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
> > > -------
> > >  1 file changed, 33 insertions(+), 17 deletions(-)
> > >
> > >
> > > @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
> > > struct mcs_spinlock *prev)
> > >                  * If pv_kick_node() changed us to vcpu_hashed, retain that
> > >                  * value so that pv_wait_head_or_lock() knows to not also
> > > try
> > >                  * to hash this lock.
> > > +                *
> > > +                * The smp_store_mb() and control dependency above will
> > > ensure
> > > +                * that state change won't happen before that.
> > > Synchronizing
> > > +                * with pv_kick_node() wrt hashing by this waiter or by the
> > > +                * lock holder is done solely by the state variable. There
> > > is
> > > +                * no other ordering requirement.
> > >                  */
> > > -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
> > > +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
> > >
> > >                 /*
> > >                  * If the locked flag is still not set after wakeup, it is
> > > a
> > > @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
> > > struct mcs_spinlock *node)
> > >          * pv_wait_node(). If OTOH this fails, the vCPU was running and
> > > will
> > >          * observe its next->locked value and advance itself.
> > >          *
> > > -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
> > > +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
> > > pv_wait_node().
> > > +        * A release barrier is used here to ensure that node->locked is
> > > +        * always set before changing the state. See comment in
> > > pv_wait_node().
> > >          */
> > > -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
> > > +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
> > > +                       != vcpu_halted)
> > >                 return;
> > >
> > > hi, Waiman
> > We can't use _release here, a full barrier is needed.
> > 
> > There is pv_kick_node vs pv_wait_head_or_lock
> > 
> > [w] l->locked = _Q_SLOW_VAL  //reordered here
> > 
> > if (READ_ONCE(pn->state) == vcpu_hashed) //False.
> > 
> >                    lp = (struct qspinlock **)1;
> > 
> > [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
> > pn);
> > pv_hash()                                                                if
> > (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
> > 
> 
> This analysis is correct, but..
> 

Hmm.. look at this again, I don't think this analysis is meaningful,
let's say the reordering didn't happen, we still got(similar to your
case):

						if (READ_ONCE(pn->state) == vcpu_hashed) // false.
						  lp = (struct qspinlock **)1;

cmpxchg(pn->state, vcpu_halted, vcpu_hashed);
						  if(!lp) {
						    lp = pv_hash(lock, pn);
WRITE_ONCE(l->locked, _Q_SLOW_VAL);
pv_hash();
						    if (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.

, right?

Actually, I think this or your case could not happen because we have

	cmpxchg(pn->state, vcpu_halted, vcpu_running);

in pv_wait_node(), which makes us either observe vcpu_hashed or set
pn->state to vcpu_running before pv_kick_node() trying to do the hash.

I may miss something subtle, but does switching back to cmpxchg() could
fix the RCU stall you observed?

Regards,
Boqun

> > Then the same lock has hashed twice but only unhashed once. So at last as
> > the hash table grows big, we hit RCU stall.
> > 
> > I hit RCU stall when I run netperf benchmark
> > 
> 
> how will a big hash table hit RCU stall? Do you have the call trace for
> your RCU stall?
> 
> Regards,
> Boqun
> 
> > thanks
> > xinhui
> > 
> > 
> > > --
> > > 1.8.3.1
> > >
> > >

[toc] | [prev] | [next] | [standalone]

#1576302

在 2017/2/8 14:09, Boqun Feng 写道:
> On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote:
>> On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
>>> 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
>>>
>>>> A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
>>>> relaxed versions to improve performance on architectures that use LL/SC.
>>>>
>>>> All the locking related cmpxchg's are replaced with the _acquire
>>>> variants:
>>>>  - pv_queued_spin_steal_lock()
>>>>  - trylock_clear_pending()
>>>>
>>>> The cmpxchg's related to hashing are replaced by either by the _release
>>>> or the _relaxed variants. See the inline comment for details.
>>>>
>>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>>>
>>>>  v1->v2:
>>>>   - Add comments in changelog and code for the rationale of the change.
>>>>
>>>> ---
>>>>  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
>>>> -------
>>>>  1 file changed, 33 insertions(+), 17 deletions(-)
>>>>
>>>>
>>>> @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
>>>> struct mcs_spinlock *prev)
>>>>                  * If pv_kick_node() changed us to vcpu_hashed, retain that
>>>>                  * value so that pv_wait_head_or_lock() knows to not also
>>>> try
>>>>                  * to hash this lock.
>>>> +                *
>>>> +                * The smp_store_mb() and control dependency above will
>>>> ensure
>>>> +                * that state change won't happen before that.
>>>> Synchronizing
>>>> +                * with pv_kick_node() wrt hashing by this waiter or by the
>>>> +                * lock holder is done solely by the state variable. There
>>>> is
>>>> +                * no other ordering requirement.
>>>>                  */
>>>> -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
>>>> +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
>>>>
>>>>                 /*
>>>>                  * If the locked flag is still not set after wakeup, it is
>>>> a
>>>> @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
>>>> struct mcs_spinlock *node)
>>>>          * pv_wait_node(). If OTOH this fails, the vCPU was running and
>>>> will
>>>>          * observe its next->locked value and advance itself.
>>>>          *
>>>> -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
>>>> +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
>>>> pv_wait_node().
>>>> +        * A release barrier is used here to ensure that node->locked is
>>>> +        * always set before changing the state. See comment in
>>>> pv_wait_node().
>>>>          */
>>>> -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
>>>> +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
>>>> +                       != vcpu_halted)
>>>>                 return;
>>>>
>>>> hi, Waiman
>>> We can't use _release here, a full barrier is needed.
>>>
>>> There is pv_kick_node vs pv_wait_head_or_lock
>>>
>>> [w] l->locked = _Q_SLOW_VAL  //reordered here
>>>
>>> if (READ_ONCE(pn->state) == vcpu_hashed) //False.
>>>
>>>                    lp = (struct qspinlock **)1;
>>>
>>> [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
>>> pn);
>>> pv_hash()                                                                if
>>> (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>>>
>>
>> This analysis is correct, but..
>>
>
> Hmm.. look at this again, I don't think this analysis is meaningful,
> let's say the reordering didn't happen, we still got(similar to your
> case):
>
> 						if (READ_ONCE(pn->state) == vcpu_hashed) // false.
> 						  lp = (struct qspinlock **)1;
>
> cmpxchg(pn->state, vcpu_halted, vcpu_hashed);
> 						  if(!lp) {
> 						    lp = pv_hash(lock, pn);
> WRITE_ONCE(l->locked, _Q_SLOW_VAL);
> pv_hash();
> 						    if (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>
> , right?
>
> Actually, I think this or your case could not happen because we have
>
> 	cmpxchg(pn->state, vcpu_halted, vcpu_running);
>
> in pv_wait_node(), which makes us either observe vcpu_hashed or set
> pn->state to vcpu_running before pv_kick_node() trying to do the hash.
>
yep, there is still a race. We have to fix it.
so I think
we must check old = xchg(&l->locked, _Q_SLOW_VAL)
if (old  == 0)
do something
else if (old  == _Q_SLOW_VAL)
do something else

> I may miss something subtle, but does switching back to cmpxchg() could
> fix the RCU stall you observed?
>
yes, just fix this cmpxchg and then no RCU stall.

> Regards,
> Boqun
>
>>> Then the same lock has hashed twice but only unhashed once. So at last as
>>> the hash table grows big, we hit RCU stall.
>>>
>>> I hit RCU stall when I run netperf benchmark
>>>
>>
>> how will a big hash table hit RCU stall? Do you have the call trace for
>> your RCU stall?
>>
maybe too many time on hashing? I am not sure.

>> Regards,
>> Boqun
>>
>>> thanks
>>> xinhui
>>>
>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>
>

[toc] | [prev] | [next] | [standalone]

#1576303

在 2017/2/8 14:09, Boqun Feng 写道:
> On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote:
>> On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
>>> 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
>>>
>>>> A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
>>>> relaxed versions to improve performance on architectures that use LL/SC.
>>>>
>>>> All the locking related cmpxchg's are replaced with the _acquire
>>>> variants:
>>>>  - pv_queued_spin_steal_lock()
>>>>  - trylock_clear_pending()
>>>>
>>>> The cmpxchg's related to hashing are replaced by either by the _release
>>>> or the _relaxed variants. See the inline comment for details.
>>>>
>>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>>>
>>>>  v1->v2:
>>>>   - Add comments in changelog and code for the rationale of the change.
>>>>
>>>> ---
>>>>  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
>>>> -------
>>>>  1 file changed, 33 insertions(+), 17 deletions(-)
>>>>
>>>>
>>>> @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
>>>> struct mcs_spinlock *prev)
>>>>                  * If pv_kick_node() changed us to vcpu_hashed, retain that
>>>>                  * value so that pv_wait_head_or_lock() knows to not also
>>>> try
>>>>                  * to hash this lock.
>>>> +                *
>>>> +                * The smp_store_mb() and control dependency above will
>>>> ensure
>>>> +                * that state change won't happen before that.
>>>> Synchronizing
>>>> +                * with pv_kick_node() wrt hashing by this waiter or by the
>>>> +                * lock holder is done solely by the state variable. There
>>>> is
>>>> +                * no other ordering requirement.
>>>>                  */
>>>> -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
>>>> +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
>>>>
>>>>                 /*
>>>>                  * If the locked flag is still not set after wakeup, it is
>>>> a
>>>> @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
>>>> struct mcs_spinlock *node)
>>>>          * pv_wait_node(). If OTOH this fails, the vCPU was running and
>>>> will
>>>>          * observe its next->locked value and advance itself.
>>>>          *
>>>> -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
>>>> +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
>>>> pv_wait_node().
>>>> +        * A release barrier is used here to ensure that node->locked is
>>>> +        * always set before changing the state. See comment in
>>>> pv_wait_node().
>>>>          */
>>>> -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
>>>> +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
>>>> +                       != vcpu_halted)
>>>>                 return;
>>>>
>>>> hi, Waiman
>>> We can't use _release here, a full barrier is needed.
>>>
>>> There is pv_kick_node vs pv_wait_head_or_lock
>>>
>>> [w] l->locked = _Q_SLOW_VAL  //reordered here
>>>
>>> if (READ_ONCE(pn->state) == vcpu_hashed) //False.
>>>
>>>                    lp = (struct qspinlock **)1;
>>>
>>> [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
>>> pn);
>>> pv_hash()                                                                if
>>> (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>>>
>>
>> This analysis is correct, but..
>>
>
> Hmm.. look at this again, I don't think this analysis is meaningful,
> let's say the reordering didn't happen, we still got(similar to your
> case):
>
> 						if (READ_ONCE(pn->state) == vcpu_hashed) // false.
> 						  lp = (struct qspinlock **)1;
>
> cmpxchg(pn->state, vcpu_halted, vcpu_hashed);
> 						  if(!lp) {
> 						    lp = pv_hash(lock, pn);
> WRITE_ONCE(l->locked, _Q_SLOW_VAL);
> pv_hash();
> 						    if (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>
> , right?
>
> Actually, I think this or your case could not happen because we have
>
> 	cmpxchg(pn->state, vcpu_halted, vcpu_running);
>
> in pv_wait_node(), which makes us either observe vcpu_hashed or set
> pn->state to vcpu_running before pv_kick_node() trying to do the hash.
>
yep, there is still a race. We have to fix it.
so I think
we must check old = xchg(&l->locked, _Q_SLOW_VAL)
if (old  == 0)
do something
else if (old  == _Q_SLOW_VAL)
do something else

> I may miss something subtle, but does switching back to cmpxchg() could
> fix the RCU stall you observed?
>
yes, just fix this cmpxchg and then no RCU stall.

> Regards,
> Boqun
>
>>> Then the same lock has hashed twice but only unhashed once. So at last as
>>> the hash table grows big, we hit RCU stall.
>>>
>>> I hit RCU stall when I run netperf benchmark
>>>
>>
>> how will a big hash table hit RCU stall? Do you have the call trace for
>> your RCU stall?
>>
>> Regards,
>> Boqun
>>
>>> thanks
>>> xinhui
>>>
>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>
>

[toc] | [prev] | [next] | [standalone]

#1576308

在 2017/2/8 14:09, Boqun Feng 写道:
> On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote:
>> On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
>>> 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
>>>
>>>> A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
>>>> relaxed versions to improve performance on architectures that use LL/SC.
>>>>
>>>> All the locking related cmpxchg's are replaced with the _acquire
>>>> variants:
>>>>  - pv_queued_spin_steal_lock()
>>>>  - trylock_clear_pending()
>>>>
>>>> The cmpxchg's related to hashing are replaced by either by the _release
>>>> or the _relaxed variants. See the inline comment for details.
>>>>
>>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>>>
>>>>  v1->v2:
>>>>   - Add comments in changelog and code for the rationale of the change.
>>>>
>>>> ---
>>>>  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
>>>> -------
>>>>  1 file changed, 33 insertions(+), 17 deletions(-)
>>>>
>>>>
>>>> @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
>>>> struct mcs_spinlock *prev)
>>>>                  * If pv_kick_node() changed us to vcpu_hashed, retain that
>>>>                  * value so that pv_wait_head_or_lock() knows to not also
>>>> try
>>>>                  * to hash this lock.
>>>> +                *
>>>> +                * The smp_store_mb() and control dependency above will
>>>> ensure
>>>> +                * that state change won't happen before that.
>>>> Synchronizing
>>>> +                * with pv_kick_node() wrt hashing by this waiter or by the
>>>> +                * lock holder is done solely by the state variable. There
>>>> is
>>>> +                * no other ordering requirement.
>>>>                  */
>>>> -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
>>>> +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
>>>>
>>>>                 /*
>>>>                  * If the locked flag is still not set after wakeup, it is
>>>> a
>>>> @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
>>>> struct mcs_spinlock *node)
>>>>          * pv_wait_node(). If OTOH this fails, the vCPU was running and
>>>> will
>>>>          * observe its next->locked value and advance itself.
>>>>          *
>>>> -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
>>>> +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
>>>> pv_wait_node().
>>>> +        * A release barrier is used here to ensure that node->locked is
>>>> +        * always set before changing the state. See comment in
>>>> pv_wait_node().
>>>>          */
>>>> -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
>>>> +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
>>>> +                       != vcpu_halted)
>>>>                 return;
>>>>
>>>> hi, Waiman
>>> We can't use _release here, a full barrier is needed.
>>>
>>> There is pv_kick_node vs pv_wait_head_or_lock
>>>
>>> [w] l->locked = _Q_SLOW_VAL  //reordered here
>>>
>>> if (READ_ONCE(pn->state) == vcpu_hashed) //False.
>>>
>>>                    lp = (struct qspinlock **)1;
>>>
>>> [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
>>> pn);
>>> pv_hash()                                                                if
>>> (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>>>
>>
>> This analysis is correct, but..
>>
>
> Hmm.. look at this again, I don't think this analysis is meaningful,
> let's say the reordering didn't happen, we still got(similar to your
> case):
>
but there is
						cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);

> 						if (READ_ONCE(pn->state) == vcpu_hashed) // false.
> 						  lp = (struct qspinlock **)1;
>
> cmpxchg(pn->state, vcpu_halted, vcpu_hashed);
this cmpxchg will observe the cmpxchg_relaxed above, so this cmpxchg will fail as pn->state is vcpu_running.
No bug here..

> 						  if(!lp) {
> 						    lp = pv_hash(lock, pn);
> WRITE_ONCE(l->locked, _Q_SLOW_VAL);
> pv_hash();
> 						    if (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
>
> , right?

>
> Actually, I think this or your case could not happen because we have
>
> 	cmpxchg(pn->state, vcpu_halted, vcpu_running);
>
> in pv_wait_node(), which makes us either observe vcpu_hashed or set
> pn->state to vcpu_running before pv_kick_node() trying to do the hash.
>
> I may miss something subtle, but does switching back to cmpxchg() could
> fix the RCU stall you observed?
>
> Regards,
> Boqun
>
>>> Then the same lock has hashed twice but only unhashed once. So at last as
>>> the hash table grows big, we hit RCU stall.
>>>
>>> I hit RCU stall when I run netperf benchmark
>>>
>>
>> how will a big hash table hit RCU stall? Do you have the call trace for
>> your RCU stall?
>>
>> Regards,
>> Boqun
>>
>>> thanks
>>> xinhui
>>>
>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>
>

[toc] | [prev] | [next] | [standalone]

#1576309

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 08, 2017 at 03:09:33PM +0800, Pan Xinhui wrote:
> 
> 
> 在 2017/2/8 14:09, Boqun Feng 写道:
> > On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote:
> > > On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote:
> > > > 2016-12-26 4:26 GMT+08:00 Waiman Long <longman@redhat.com>:
> > > > 
> > > > > A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
> > > > > relaxed versions to improve performance on architectures that use LL/SC.
> > > > > 
> > > > > All the locking related cmpxchg's are replaced with the _acquire
> > > > > variants:
> > > > >  - pv_queued_spin_steal_lock()
> > > > >  - trylock_clear_pending()
> > > > > 
> > > > > The cmpxchg's related to hashing are replaced by either by the _release
> > > > > or the _relaxed variants. See the inline comment for details.
> > > > > 
> > > > > Signed-off-by: Waiman Long <longman@redhat.com>
> > > > > 
> > > > >  v1->v2:
> > > > >   - Add comments in changelog and code for the rationale of the change.
> > > > > 
> > > > > ---
> > > > >  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------
> > > > > -------
> > > > >  1 file changed, 33 insertions(+), 17 deletions(-)
> > > > > 
> > > > > 
> > > > > @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node,
> > > > > struct mcs_spinlock *prev)
> > > > >                  * If pv_kick_node() changed us to vcpu_hashed, retain that
> > > > >                  * value so that pv_wait_head_or_lock() knows to not also
> > > > > try
> > > > >                  * to hash this lock.
> > > > > +                *
> > > > > +                * The smp_store_mb() and control dependency above will
> > > > > ensure
> > > > > +                * that state change won't happen before that.
> > > > > Synchronizing
> > > > > +                * with pv_kick_node() wrt hashing by this waiter or by the
> > > > > +                * lock holder is done solely by the state variable. There
> > > > > is
> > > > > +                * no other ordering requirement.
> > > > >                  */
> > > > > -               cmpxchg(&pn->state, vcpu_halted, vcpu_running);
> > > > > +               cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
> > > > > 
> > > > >                 /*
> > > > >                  * If the locked flag is still not set after wakeup, it is
> > > > > a
> > > > > @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock,
> > > > > struct mcs_spinlock *node)
> > > > >          * pv_wait_node(). If OTOH this fails, the vCPU was running and
> > > > > will
> > > > >          * observe its next->locked value and advance itself.
> > > > >          *
> > > > > -        * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
> > > > > +        * Matches with smp_store_mb() and cmpxchg_relaxed() in
> > > > > pv_wait_node().
> > > > > +        * A release barrier is used here to ensure that node->locked is
> > > > > +        * always set before changing the state. See comment in
> > > > > pv_wait_node().
> > > > >          */
> > > > > -       if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
> > > > > +       if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed)
> > > > > +                       != vcpu_halted)
> > > > >                 return;
> > > > > 
> > > > > hi, Waiman
> > > > We can't use _release here, a full barrier is needed.
> > > > 
> > > > There is pv_kick_node vs pv_wait_head_or_lock
> > > > 
> > > > [w] l->locked = _Q_SLOW_VAL  //reordered here
> > > > 
> > > > if (READ_ONCE(pn->state) == vcpu_hashed) //False.
> > > > 
> > > >                    lp = (struct qspinlock **)1;
> > > > 
> > > > [STORE] pn->state = vcpu_hashed                        lp = pv_hash(lock,
> > > > pn);
> > > > pv_hash()                                                                if
> > > > (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed.
> > > > 
> > > 
> > > This analysis is correct, but..
> > > 
> > 
> > Hmm.. look at this again, I don't think this analysis is meaningful,
> > let's say the reordering didn't happen, we still got(similar to your
> > case):
> > 
> but there is
> 						cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running);
> 
> > 						if (READ_ONCE(pn->state) == vcpu_hashed) // false.
> > 						  lp = (struct qspinlock **)1;
> > 
> > cmpxchg(pn->state, vcpu_halted, vcpu_hashed);
> this cmpxchg will observe the cmpxchg_relaxed above, so this cmpxchg will fail as pn->state is vcpu_running.
> No bug here..
> 

And we got the same guarantee if we use cmpxchg_release(), no?

Regards,
Boqun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web