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

Groups > linux.kernel > #1185993 > unrolled thread

[PATCH v2 5/7] locking/qrwlock: make use of acquire/release/relaxed atomics

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

  [PATCH v2 5/7] locking/qrwlock: make use of acquire/release/relaxed atomics Will Deacon <will.deacon@arm.com> - 2015-07-16 17:40 +0200
    Re: [PATCH v2 5/7] locking/qrwlock: make use of  acquire/release/relaxed atomics Peter Zijlstra <peterz@infradead.org> - 2015-07-16 19:00 +0200
      Re: [PATCH v2 5/7] locking/qrwlock: make use of  acquire/release/relaxed atomics Will Deacon <will.deacon@arm.com> - 2015-07-16 20:20 +0200

#1185993 — [PATCH v2 5/7] locking/qrwlock: make use of acquire/release/relaxed atomics

The qrwlock implementation is slightly heavy in its use of memory
barriers, mainly through the use of cmpxchg and _return atomics, which
imply full barrier semantics.

This patch modifies the qrwlock code to use the more relaxed atomic
routines so that we can reduce the unnecessary barrier overhead on
weakly-ordered architectures.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/asm-generic/qrwlock.h | 13 ++++++-------
 kernel/locking/qrwlock.c      | 12 ++++++------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 930920501e33..6ab846bf6413 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -68,7 +68,7 @@ static inline int queued_read_trylock(struct qrwlock *lock)
 
 	cnts = atomic_read(&lock->cnts);
 	if (likely(!(cnts & _QW_WMASK))) {
-		cnts = (u32)atomic_add_return(_QR_BIAS, &lock->cnts);
+		cnts = (u32)atomic_add_return_acquire(_QR_BIAS, &lock->cnts);
 		if (likely(!(cnts & _QW_WMASK)))
 			return 1;
 		atomic_sub(_QR_BIAS, &lock->cnts);
@@ -89,8 +89,8 @@ static inline int queued_write_trylock(struct qrwlock *lock)
 	if (unlikely(cnts))
 		return 0;
 
-	return likely(atomic_cmpxchg(&lock->cnts,
-				     cnts, cnts | _QW_LOCKED) == cnts);
+	return likely(atomic_cmpxchg_acquire(&lock->cnts,
+					     cnts, cnts | _QW_LOCKED) == cnts);
 }
 /**
  * queued_read_lock - acquire read lock of a queue rwlock
@@ -100,7 +100,7 @@ static inline void queued_read_lock(struct qrwlock *lock)
 {
 	u32 cnts;
 
-	cnts = atomic_add_return(_QR_BIAS, &lock->cnts);
+	cnts = atomic_add_return_acquire(_QR_BIAS, &lock->cnts);
 	if (likely(!(cnts & _QW_WMASK)))
 		return;
 
@@ -115,7 +115,7 @@ static inline void queued_read_lock(struct qrwlock *lock)
 static inline void queued_write_lock(struct qrwlock *lock)
 {
 	/* Optimize for the unfair lock case where the fair flag is 0. */
-	if (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0)
+	if (atomic_cmpxchg_acquire(&lock->cnts, 0, _QW_LOCKED) == 0)
 		return;
 
 	queued_write_lock_slowpath(lock);
@@ -130,8 +130,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
 	/*
 	 * Atomically decrement the reader count
 	 */
-	smp_mb__before_atomic();
-	atomic_sub(_QR_BIAS, &lock->cnts);
+	(void)atomic_sub_return_release(_QR_BIAS, &lock->cnts);
 }
 
 /**
diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index a71bb3541880..879c8fab7bea 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -36,7 +36,7 @@ rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
 {
 	while ((cnts & _QW_WMASK) == _QW_LOCKED) {
 		cpu_relax_lowlatency();
-		cnts = smp_load_acquire((u32 *)&lock->cnts);
+		cnts = atomic_read_acquire(&lock->cnts);
 	}
 }
 
@@ -78,7 +78,7 @@ void queued_read_lock_slowpath(struct qrwlock *lock, u32 cnts)
 	while (atomic_read(&lock->cnts) & _QW_WMASK)
 		cpu_relax_lowlatency();
 
-	cnts = atomic_add_return(_QR_BIAS, &lock->cnts) - _QR_BIAS;
+	cnts = atomic_add_return_relaxed(_QR_BIAS, &lock->cnts) - _QR_BIAS;
 	rspin_until_writer_unlock(lock, cnts);
 
 	/*
@@ -101,7 +101,7 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
 
 	/* Try to acquire the lock directly if no reader is present */
 	if (!atomic_read(&lock->cnts) &&
-	    (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0))
+	    (atomic_cmpxchg_acquire(&lock->cnts, 0, _QW_LOCKED) == 0))
 		goto unlock;
 
 	/*
@@ -110,7 +110,7 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
 	 */
 	for (;;) {
 		if (!READ_ONCE(lock->wmode) &&
-		   (cmpxchg(&lock->wmode, 0, _QW_WAITING) == 0))
+		   (cmpxchg_relaxed(&lock->wmode, 0, _QW_WAITING) == 0))
 			break;
 
 		cpu_relax_lowlatency();
@@ -120,8 +120,8 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
 	for (;;) {
 		cnts = atomic_read(&lock->cnts);
 		if ((cnts == _QW_WAITING) &&
-		    (atomic_cmpxchg(&lock->cnts, _QW_WAITING,
-				    _QW_LOCKED) == _QW_WAITING))
+		    (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING,
+					    _QW_LOCKED) == _QW_WAITING))
 			break;
 
 		cpu_relax_lowlatency();
-- 
2.1.4

--
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]

#1186071 — Re: [PATCH v2 5/7] locking/qrwlock: make use of acquire/release/relaxed atomics

On Thu, Jul 16, 2015 at 04:32:36PM +0100, Will Deacon wrote:
> @@ -130,8 +130,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>  	/*
>  	 * Atomically decrement the reader count
>  	 */
> -	smp_mb__before_atomic();
> -	atomic_sub(_QR_BIAS, &lock->cnts);
> +	(void)atomic_sub_return_release(_QR_BIAS, &lock->cnts);
>  }
>  
>  /**

This one will actually cause different code on x86; I think its still
fine though. LOCK XADD should not be (much) slower than LOCK SUB.

> diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> index a71bb3541880..879c8fab7bea 100644
> --- a/kernel/locking/qrwlock.c
> +++ b/kernel/locking/qrwlock.c
> @@ -36,7 +36,7 @@ rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
>  {
>  	while ((cnts & _QW_WMASK) == _QW_LOCKED) {
>  		cpu_relax_lowlatency();
> -		cnts = smp_load_acquire((u32 *)&lock->cnts);
> +		cnts = atomic_read_acquire(&lock->cnts);
>  	}
>  }

It might make sense to add comments to the users of this function that
actually rely on the _acquire semantics, I had to double check that :-)


But otherwise that all looks good.
--
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]

#1186176 — Re: [PATCH v2 5/7] locking/qrwlock: make use of acquire/release/relaxed atomics

On Thu, Jul 16, 2015 at 05:59:03PM +0100, Peter Zijlstra wrote:
> On Thu, Jul 16, 2015 at 04:32:36PM +0100, Will Deacon wrote:
> > @@ -130,8 +130,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
> >  	/*
> >  	 * Atomically decrement the reader count
> >  	 */
> > -	smp_mb__before_atomic();
> > -	atomic_sub(_QR_BIAS, &lock->cnts);
> > +	(void)atomic_sub_return_release(_QR_BIAS, &lock->cnts);
> >  }
> >  
> >  /**
> 
> This one will actually cause different code on x86; I think its still
> fine though. LOCK XADD should not be (much) slower than LOCK SUB.

Yeah, I wondered whether introduced atomic_sub_release etc was worth the
hassle and decided against it for now.

> > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> > index a71bb3541880..879c8fab7bea 100644
> > --- a/kernel/locking/qrwlock.c
> > +++ b/kernel/locking/qrwlock.c
> > @@ -36,7 +36,7 @@ rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
> >  {
> >  	while ((cnts & _QW_WMASK) == _QW_LOCKED) {
> >  		cpu_relax_lowlatency();
> > -		cnts = smp_load_acquire((u32 *)&lock->cnts);
> > +		cnts = atomic_read_acquire(&lock->cnts);
> >  	}
> >  }
> 
> It might make sense to add comments to the users of this function that
> actually rely on the _acquire semantics, I had to double check that :-)

Good point, I'll add those.

> But otherwise that all looks good.

Cheers. I'll send a v3 next week with your comments addressed. Pending
any objection, I guess this could be merged via -tip with the exception
of the ARM patch? FWIW, I plan to port arm64 once I've got my pending
asm/atomic.h rework queued.

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