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

Groups > linux.kernel > #1246017 > unrolled thread

Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in native code

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 v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in  native code Peter Zijlstra <peterz@infradead.org> - 2015-10-13 20:10 +0200
    Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops  in native code Waiman Long <waiman.long@hpe.com> - 2015-10-13 22:40 +0200
      Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in  native code Peter Zijlstra <peterz@infradead.org> - 2015-10-13 22:50 +0200
    Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in  native code Will Deacon <will.deacon@arm.com> - 2015-10-14 11:40 +0200

#1246017 — Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in native code

On Tue, Sep 22, 2015 at 04:50:40PM -0400, Waiman Long wrote:
> This patch replaces the cmpxchg() and xchg() calls in the native
> qspinlock code with more relaxed versions of those calls to enable
> other architectures to adopt queued spinlocks with less performance
> overhead.

> @@ -62,7 +63,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
>  static __always_inline int queued_spin_trylock(struct qspinlock *lock)
>  {
>  	if (!atomic_read(&lock->val) &&
> -	   (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> +	   (atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL) == 0))
>  		return 1;
>  	return 0;
>  }
> @@ -77,7 +78,7 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
>  {
>  	u32 val;
>  
> -	val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> +	val = atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL);
>  	if (likely(val == 0))
>  		return;
>  	queued_spin_lock_slowpath(lock, val);

> @@ -319,7 +329,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>  		if (val == new)
>  			new |= _Q_PENDING_VAL;
>  
> -		old = atomic_cmpxchg(&lock->val, val, new);
> +		old = atomic_cmpxchg_acquire(&lock->val, val, new);
>  		if (old == val)
>  			break;
>  

So given recent discussion, all this _release/_acquire stuff is starting
to worry me.

So we've not declared if they should be RCsc or RCpc, and given this
patch (and the previous ones) these lock primitives turn into RCpc if
the atomic primitives are RCpc.

So far only the proposed PPC implementation is RCpc -- and their current
spinlock implementation is also RCpc, but that is a point of discussion.

Just saying..

Also, I think we should annotate the control dependencies in these
things.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1246126 — Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in native code

On 10/13/2015 02:02 PM, Peter Zijlstra wrote:
> On Tue, Sep 22, 2015 at 04:50:40PM -0400, Waiman Long wrote:
>> This patch replaces the cmpxchg() and xchg() calls in the native
>> qspinlock code with more relaxed versions of those calls to enable
>> other architectures to adopt queued spinlocks with less performance
>> overhead.
>> @@ -62,7 +63,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
>>   static __always_inline int queued_spin_trylock(struct qspinlock *lock)
>>   {
>>   	if (!atomic_read(&lock->val)&&
>> -	   (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) == 0))
>> +	   (atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL) == 0))
>>   		return 1;
>>   	return 0;
>>   }
>> @@ -77,7 +78,7 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
>>   {
>>   	u32 val;
>>
>> -	val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
>> +	val = atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL);
>>   	if (likely(val == 0))
>>   		return;
>>   	queued_spin_lock_slowpath(lock, val);
>> @@ -319,7 +329,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>>   		if (val == new)
>>   			new |= _Q_PENDING_VAL;
>>
>> -		old = atomic_cmpxchg(&lock->val, val, new);
>> +		old = atomic_cmpxchg_acquire(&lock->val, val, new);
>>   		if (old == val)
>>   			break;
>>
> So given recent discussion, all this _release/_acquire stuff is starting
> to worry me.
>
> So we've not declared if they should be RCsc or RCpc, and given this
> patch (and the previous ones) these lock primitives turn into RCpc if
> the atomic primitives are RCpc.
>
> So far only the proposed PPC implementation is RCpc -- and their current
> spinlock implementation is also RCpc, but that is a point of discussion.
>
> Just saying..

Davidlohr's patches to make similar changes in other locking code will 
also have this issue. Anyway, the goal of this patch is to make the 
generic qspinlock code less costly when ported to other architectures. 
This change will have no effect on the x86 architecture which is the 
only one using qspinlock at the moment.

>
> Also, I think we should annotate the control dependencies in these
> things.

Will do.

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1246136

On Tue, Oct 13, 2015 at 04:38:19PM -0400, Waiman Long wrote:
> On 10/13/2015 02:02 PM, Peter Zijlstra wrote:
> >On Tue, Sep 22, 2015 at 04:50:40PM -0400, Waiman Long wrote:
> >>This patch replaces the cmpxchg() and xchg() calls in the native
> >>qspinlock code with more relaxed versions of those calls to enable
> >>other architectures to adopt queued spinlocks with less performance
> >>overhead.
> >>@@ -62,7 +63,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
> >>  static __always_inline int queued_spin_trylock(struct qspinlock *lock)
> >>  {
> >>  	if (!atomic_read(&lock->val)&&
> >>-	   (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> >>+	   (atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> >>  		return 1;
> >>  	return 0;
> >>  }
> >>@@ -77,7 +78,7 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
> >>  {
> >>  	u32 val;
> >>
> >>-	val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> >>+	val = atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL);
> >>  	if (likely(val == 0))
> >>  		return;
> >>  	queued_spin_lock_slowpath(lock, val);
> >>@@ -319,7 +329,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> >>  		if (val == new)
> >>  			new |= _Q_PENDING_VAL;
> >>
> >>-		old = atomic_cmpxchg(&lock->val, val, new);
> >>+		old = atomic_cmpxchg_acquire(&lock->val, val, new);
> >>  		if (old == val)
> >>  			break;
> >>
> >So given recent discussion, all this _release/_acquire stuff is starting
> >to worry me.
> >
> >So we've not declared if they should be RCsc or RCpc, and given this
> >patch (and the previous ones) these lock primitives turn into RCpc if
> >the atomic primitives are RCpc.
> >
> >So far only the proposed PPC implementation is RCpc -- and their current
> >spinlock implementation is also RCpc, but that is a point of discussion.
> >
> >Just saying..
> 
> Davidlohr's patches to make similar changes in other locking code will also
> have this issue. 

Yes, I only fully appreciated the RCpc pain last week :/

> Anyway, the goal of this patch is to make the generic
> qspinlock code less costly when ported to other architectures. 

As long as we stay away from PPC this will be fine ;-) Luckily they are
unlikely to start using it since their LPAR hypervisor thingy isn't
really co-operative.

> This change
> will have no effect on the x86 architecture which is the only one using

I know ARM and ARGH64 will want to start using this, luckily both are
RCsc so no worries there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1246462

On Tue, Oct 13, 2015 at 08:02:25PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 22, 2015 at 04:50:40PM -0400, Waiman Long wrote:
> > This patch replaces the cmpxchg() and xchg() calls in the native
> > qspinlock code with more relaxed versions of those calls to enable
> > other architectures to adopt queued spinlocks with less performance
> > overhead.
> 
> > @@ -62,7 +63,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
> >  static __always_inline int queued_spin_trylock(struct qspinlock *lock)
> >  {
> >  	if (!atomic_read(&lock->val) &&
> > -	   (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> > +	   (atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> >  		return 1;
> >  	return 0;
> >  }
> > @@ -77,7 +78,7 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
> >  {
> >  	u32 val;
> >  
> > -	val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> > +	val = atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL);
> >  	if (likely(val == 0))
> >  		return;
> >  	queued_spin_lock_slowpath(lock, val);
> 
> > @@ -319,7 +329,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> >  		if (val == new)
> >  			new |= _Q_PENDING_VAL;
> >  
> > -		old = atomic_cmpxchg(&lock->val, val, new);
> > +		old = atomic_cmpxchg_acquire(&lock->val, val, new);
> >  		if (old == val)
> >  			break;
> >  
> 
> So given recent discussion, all this _release/_acquire stuff is starting
> to worry me.
> 
> So we've not declared if they should be RCsc or RCpc, and given this
> patch (and the previous ones) these lock primitives turn into RCpc if
> the atomic primitives are RCpc.

Our spinlocks are currently RCpc and I think these really should match.

> So far only the proposed PPC implementation is RCpc -- and their current
> spinlock implementation is also RCpc, but that is a point of discussion.
> 
> Just saying..

Well, PPC already made that choice for their spinlocks, so I don't
necessarily think we should worry too much here. They get to choose
between a ~5% performance hit (iirc) or a higher potential for subtle
locking issues. That said, I don't believe the RCpc/RCsc choice actually
affects common locking paradigms (so far, we're only aware of RCU needing
to do something different on PPC).

If we end up getting swamped with horrible bug reports from PPC users,
then we can add a TAINT_RCPC ;) (or just strengthen their implementation).

> Also, I think we should annotate the control dependencies in these
> things.

Yes, please.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web