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

Groups > linux.kernel > #1370594 > unrolled thread

[RFC][PATCH 2/3] locking/qrwlock: Use smp_cond_load_acquire()

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

  [RFC][PATCH 2/3] locking/qrwlock: Use smp_cond_load_acquire() Peter Zijlstra <peterz@infradead.org> - 2016-04-04 14:40 +0200
    Re: [RFC][PATCH 2/3] locking/qrwlock: Use smp_cond_load_acquire() Davidlohr Bueso <dave@stgolabs.net> - 2016-04-12 07:00 +0200
      Re: [RFC][PATCH 2/3] locking/qrwlock: Use smp_cond_load_acquire() Waiman Long <waiman.long@hpe.com> - 2016-04-12 18:50 +0200

#1370594 — [RFC][PATCH 2/3] locking/qrwlock: Use smp_cond_load_acquire()

Use smp_cond_load_acquire() to make better use of the hardware
assisted 'spin' wait on arm64.

Arguably the second hunk is the more horrid abuse possible, but
avoids having to use cmpwait (see next patch) directly. Also, this
makes 'clever' (ab)use of the cond+rmb acquire to omit the acquire
from cmpxchg().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/locking/qrwlock.c |   18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -53,10 +53,7 @@ struct __qrwlock {
 static __always_inline void
 rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
 {
-	while ((cnts & _QW_WMASK) == _QW_LOCKED) {
-		cpu_relax_lowlatency();
-		cnts = atomic_read_acquire(&lock->cnts);
-	}
+	smp_cond_load_acquire(&lock->cnts.counter, (VAL & _QW_WMASK) != _QW_LOCKED);
 }
 
 /**
@@ -109,8 +106,6 @@ EXPORT_SYMBOL(queued_read_lock_slowpath)
  */
 void queued_write_lock_slowpath(struct qrwlock *lock)
 {
-	u32 cnts;
-
 	/* Put the writer into the wait queue */
 	arch_spin_lock(&lock->wait_lock);
 
@@ -134,15 +129,10 @@ void queued_write_lock_slowpath(struct q
 	}
 
 	/* When no more readers, set the locked flag */
-	for (;;) {
-		cnts = atomic_read(&lock->cnts);
-		if ((cnts == _QW_WAITING) &&
-		    (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING,
-					    _QW_LOCKED) == _QW_WAITING))
-			break;
+	smp_cond_load_acquire(&lock->cnts.counter,
+		(VAL == _QW_WAITING) &&
+		atomic_cmpxchg_relaxed(&lock->cnts, _QW_WAITING, _QW_LOCKED) == _QW_WAITING);
 
-		cpu_relax_lowlatency();
-	}
 unlock:
 	arch_spin_unlock(&lock->wait_lock);
 }

[toc] | [next] | [standalone]

#1376459

On Mon, 04 Apr 2016, Peter Zijlstra wrote:

>Use smp_cond_load_acquire() to make better use of the hardware
>assisted 'spin' wait on arm64.
>
>Arguably the second hunk is the more horrid abuse possible, but
>avoids having to use cmpwait (see next patch) directly. Also, this
>makes 'clever' (ab)use of the cond+rmb acquire to omit the acquire
>from cmpxchg().
>
>Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>---
> kernel/locking/qrwlock.c |   18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
>--- a/kernel/locking/qrwlock.c
>+++ b/kernel/locking/qrwlock.c
>@@ -53,10 +53,7 @@ struct __qrwlock {
> static __always_inline void
> rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
> {
>-	while ((cnts & _QW_WMASK) == _QW_LOCKED) {
>-		cpu_relax_lowlatency();
>-		cnts = atomic_read_acquire(&lock->cnts);
>-	}
>+	smp_cond_load_acquire(&lock->cnts.counter, (VAL & _QW_WMASK) != _QW_LOCKED);
> }
>
> /**
>@@ -109,8 +106,6 @@ EXPORT_SYMBOL(queued_read_lock_slowpath)
>  */
> void queued_write_lock_slowpath(struct qrwlock *lock)
> {
>-	u32 cnts;
>-
> 	/* Put the writer into the wait queue */
> 	arch_spin_lock(&lock->wait_lock);
>
>@@ -134,15 +129,10 @@ void queued_write_lock_slowpath(struct q
> 	}
>
> 	/* When no more readers, set the locked flag */
>-	for (;;) {
>-		cnts = atomic_read(&lock->cnts);
>-		if ((cnts == _QW_WAITING) &&
>-		    (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING,
>-					    _QW_LOCKED) == _QW_WAITING))
>-			break;
>+	smp_cond_load_acquire(&lock->cnts.counter,
>+		(VAL == _QW_WAITING) &&
>+		atomic_cmpxchg_relaxed(&lock->cnts, _QW_WAITING, _QW_LOCKED) == _QW_WAITING);
>
>-		cpu_relax_lowlatency();

You would need some variant for cpu_relax_lowlatency otherwise you'll be hurting s390, no?
fwiw back when I was looking at this, I recall thinking about possibly introducing
smp_cond_acquire_lowlatency but never got around to it.

Thanks,
Davidlohr

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

#1377014

On 04/12/2016 12:58 AM, Davidlohr Bueso wrote:
> On Mon, 04 Apr 2016, Peter Zijlstra wrote:
>
>> Use smp_cond_load_acquire() to make better use of the hardware
>> assisted 'spin' wait on arm64.
>>
>> Arguably the second hunk is the more horrid abuse possible, but
>> avoids having to use cmpwait (see next patch) directly. Also, this
>> makes 'clever' (ab)use of the cond+rmb acquire to omit the acquire
>> from cmpxchg().
>>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> ---
>> kernel/locking/qrwlock.c |   18 ++++--------------
>> 1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> --- a/kernel/locking/qrwlock.c
>> +++ b/kernel/locking/qrwlock.c
>> @@ -53,10 +53,7 @@ struct __qrwlock {
>> static __always_inline void
>> rspin_until_writer_unlock(struct qrwlock *lock, u32 cnts)
>> {
>> -    while ((cnts & _QW_WMASK) == _QW_LOCKED) {
>> -        cpu_relax_lowlatency();
>> -        cnts = atomic_read_acquire(&lock->cnts);
>> -    }
>> +    smp_cond_load_acquire(&lock->cnts.counter, (VAL & _QW_WMASK) != 
>> _QW_LOCKED);
>> }
>>
>> /**
>> @@ -109,8 +106,6 @@ EXPORT_SYMBOL(queued_read_lock_slowpath)
>>  */
>> void queued_write_lock_slowpath(struct qrwlock *lock)
>> {
>> -    u32 cnts;
>> -
>>     /* Put the writer into the wait queue */
>>     arch_spin_lock(&lock->wait_lock);
>>
>> @@ -134,15 +129,10 @@ void queued_write_lock_slowpath(struct q
>>     }
>>
>>     /* When no more readers, set the locked flag */
>> -    for (;;) {
>> -        cnts = atomic_read(&lock->cnts);
>> -        if ((cnts == _QW_WAITING) &&
>> -            (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING,
>> -                        _QW_LOCKED) == _QW_WAITING))
>> -            break;
>> +    smp_cond_load_acquire(&lock->cnts.counter,
>> +        (VAL == _QW_WAITING) &&
>> +        atomic_cmpxchg_relaxed(&lock->cnts, _QW_WAITING, _QW_LOCKED) 
>> == _QW_WAITING);
>>
>> -        cpu_relax_lowlatency();
>
> You would need some variant for cpu_relax_lowlatency otherwise you'll 
> be hurting s390, no?
> fwiw back when I was looking at this, I recall thinking about possibly 
> introducing
> smp_cond_acquire_lowlatency but never got around to it.
>
> Thanks,
> Davidlohr

The qrwlock is currently only used on x86 architecture. We can also come 
back to revisit this issue when other architectures that need the 
lowlatency variants are going to use qrwlock.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web