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

Groups > linux.kernel > #1586356 > unrolled thread

[PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if spinner present

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-tip 1/3] locking/rwsem: Check wait_list without lock if spinner present Waiman Long <longman@redhat.com> - 2017-02-22 19:10 +0100
    Re: [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if  spinner present Davidlohr Bueso <dave@stgolabs.net> - 2017-02-26 20:10 +0100
      Re: [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if  spinner present Waiman Long <longman@redhat.com> - 2017-02-27 16:10 +0100

#1586356 — [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if spinner present

We can safely check the wait_list to see if waiters are present without
lock when there are spinners to fall back on in case we miss a waiter.
The advantage is that we can save a pair of spin_lock/unlock calls
when the wait_list is empty. This translates to a reduction in latency
and hence slightly better performance.

On a 2-socket 36-core 72-thread x86-64 E5-2699 v3 system, a rwsem
microbenchmark was run with 36 locking threads (one/core) doing 1
million writer lock/unlock operations each, the resulting locking
rates (avg of 4 runs) on a 4.10 kernel were 7,755 Mop/s and 8,276
Mop/s without and with the patch respectively. That was an increase
of about 7%.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/rwsem-xadd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 34e727f..b5d7055 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -611,6 +611,17 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
 		 * state is consulted before reading the wait_lock.
 		 */
 		smp_rmb();
+
+		/*
+		 * Normally checking wait_list without wait_lock isn't safe
+		 * as we may miss an incoming waiter. With spinners present,
+		 * however, we have someone to fall back on in case that
+		 * happens. This can save a pair of spin_lock/unlock calls
+		 * when there is no waiter.
+		 */
+		if (list_empty(&sem->wait_list))
+			return sem;
+
 		if (!raw_spin_trylock_irqsave(&sem->wait_lock, flags))
 			return sem;
 		goto locked;
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1588379 — Re: [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if spinner present

On Wed, 22 Feb 2017, Waiman Long wrote:

>We can safely check the wait_list to see if waiters are present without
>lock when there are spinners to fall back on in case we miss a waiter.
>The advantage is that we can save a pair of spin_lock/unlock calls
>when the wait_list is empty. This translates to a reduction in latency
>and hence slightly better performance.

This benefit is only seen in (rare) situations where there are only
writers with short hold times, no? I don't really have any objection
as I doubt the additional load will have any impact on the common case,
but it would still be nice to have more data for other benchmarks where
the lock is at least shared at times -- ie: a good thing to measure is
also fault, mmap related benchmarks.

>+		/*
>+		 * Normally checking wait_list without wait_lock isn't safe
>+		 * as we may miss an incoming waiter. With spinners present,
>+		 * however, we have someone to fall back on in case that
>+		 * happens. This can save a pair of spin_lock/unlock calls
>+		 * when there is no waiter.
>+		 */

I would drop the last part regarding saving the spin_lock, it should be
evident from the code.

Thanks,
Davidlohr

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

#1588710 — Re: [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if spinner present

On 02/26/2017 01:49 PM, Davidlohr Bueso wrote:
> On Wed, 22 Feb 2017, Waiman Long wrote:
>
>> We can safely check the wait_list to see if waiters are present without
>> lock when there are spinners to fall back on in case we miss a waiter.
>> The advantage is that we can save a pair of spin_lock/unlock calls
>> when the wait_list is empty. This translates to a reduction in latency
>> and hence slightly better performance.
>
> This benefit is only seen in (rare) situations where there are only
> writers with short hold times, no? I don't really have any objection
> as I doubt the additional load will have any impact on the common case,
> but it would still be nice to have more data for other benchmarks where
> the lock is at least shared at times -- ie: a good thing to measure is
> also fault, mmap related benchmarks.

If a up_write() or up_read() coincides with a writer attempting to lock
(down_write). The unlocker may go into the wake_rwsem path even if on
one is on the wait queue. In this case, this patch can save an unneeded
spin_lock/unlock. This was what happened in the microbenchmark that I used.

The additional load shouldn't have any noticeable performance impact as
the wait_list need to be read sooner or later anyway. BTW, can you
suggest a good benchmark for testing fault, mmap related code paths?

>> +        /*
>> +         * Normally checking wait_list without wait_lock isn't safe
>> +         * as we may miss an incoming waiter. With spinners present,
>> +         * however, we have someone to fall back on in case that
>> +         * happens. This can save a pair of spin_lock/unlock calls
>> +         * when there is no waiter.
>> +         */
>
> I would drop the last part regarding saving the spin_lock, it should be
> evident from the code. 

I will do that.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web