Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Davidlohr Bueso]

Adding lkml.

On Thu, 2014-07-03 at 12:37 -0700, Davidlohr Bueso wrote:
> On Thu, 2014-07-03 at 11:50 -0700, Jason Low wrote:
> > On Wed, Jul 2, 2014 at 7:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> > > This is what the kernel profile looks like on the strided run:
> > >
> > > -  83.06%  [kernel]  [k] osq_lock
> > >    - osq_lock
> > >       - 100.00% rwsem_down_write_failed
> > >          - call_rwsem_down_write_failed
> > >             - 99.55% sys_mprotect
> > >                  tracesys
> > >                  __GI___mprotect
> > > -  12.02%  [kernel]  [k] rwsem_down_write_failed
> > 
> > Hi Dave,
> > 
> > So with no sign of rwsem_spin_on_owner(), yet with such heavy contention in
> > osq_lock, this makes me wonder if it's spending most of its time spinning
> > on !owner while a reader has the lock? (We don't set sem->owner for the readers.)
> 
> That would explain the long hold times with the memory allocation
> patterns between read and write locking described by Dave.
> 
> > If that's an issue, maybe the below is worth a test, in which we'll just
> > avoid spinning if rwsem_can_spin_on_owner() finds that there is no owner.
> > If we just had to enter the slowpath yet there is no owner, we'll be conservative
> > and assume readers have the lock.
> 
> I do worry a bit about the effects here when this is not an issue.
> Workloads that have smaller hold times could very well take a
> performance hit by blocking right away instead of wasting a few extra
> cycles just spinning.
> 
> > (David, you've tested something like this in the original patch with AIM7 and still
> > got the big performance boosts right?)
> 
> I have not, but will. I wouldn't mind sacrificing a bit of the great
> performance numbers we're getting on workloads that mostly take the lock
> for writing, if it means not being so devastating for when readers are
> in the picture. This is a major difference with mutexes wrt optimistic
> spinning.
> 
> Thanks,
> Davidlohr

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Dave Chinner]

[re-added lkml]

On Thu, Jul 03, 2014 at 11:50:20AM -0700, Jason Low wrote:
> On Wed, Jul 2, 2014 at 7:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> > This is what the kernel profile looks like on the strided run:
> >
> > -  83.06%  [kernel]  [k] osq_lock
> >    - osq_lock
> >       - 100.00% rwsem_down_write_failed
> >          - call_rwsem_down_write_failed
> >             - 99.55% sys_mprotect
> >                  tracesys
> >                  __GI___mprotect
> > -  12.02%  [kernel]  [k] rwsem_down_write_failed
> 
> Hi Dave,
> 
> So with no sign of rwsem_spin_on_owner(), yet with such heavy contention in
> osq_lock, this makes me wonder if it's spending most of its time spinning
> on !owner while a reader has the lock? (We don't set sem->owner for the readers.)
> 
> If that's an issue, maybe the below is worth a test, in which we'll just
> avoid spinning if rwsem_can_spin_on_owner() finds that there is no owner.
> If we just had to enter the slowpath yet there is no owner, we'll be conservative
> and assume readers have the lock.

That makes it quite a bit faster:

        XFS_REPAIR Summary    Fri Jul  4 10:39:32 2014

Phase           Start           End             Duration
Phase 1:        07/04 10:38:04  07/04 10:38:05  1 second
Phase 2:        07/04 10:38:05  07/04 10:38:08  3 seconds
Phase 3:        07/04 10:38:08  07/04 10:39:12  1 minute, 4 seconds
Phase 4:        07/04 10:39:12  07/04 10:39:21  9 seconds
Phase 5:        07/04 10:39:21  07/04 10:39:22  1 second
Phase 6:        07/04 10:39:22  07/04 10:39:30  8 seconds
Phase 7:        07/04 10:39:30  07/04 10:39:30  

Total run time: 1 minute, 26 seconds
done

real    1m28.504s
user    1m23.990s
sys     3m20.132s

So system time goes down massively, and speed comes up to within 30%
of the single AG run.  But it's still 2-3000 IOPS down, but it's
acceptible for the moment.

FWIW, the kernel profile ifor the multi-AG run now looks like:

  29.64%  [kernel]  [k] _raw_spin_unlock_irq
   - _raw_spin_unlock_irq
      + 35.34% __schedule
      - 34.15% call_rwsem_down_write_failed
         + 99.27% sys_mprotect
      - 30.02% call_rwsem_down_read_failed
           95.59% __do_page_fault
-  24.65%  [kernel]  [k] _raw_spin_unlock_irqrestore
   - _raw_spin_unlock_irqrestore
      - 69.38% rwsem_wake
         - call_rwsem_wake
            - 83.32% sys_mprotect
            + 15.54% __do_page_fault
      + 22.55% try_to_wake_up
+   9.77%  [kernel]  [k] default_send_IPI_mask_sequence_phys
-   3.21%  [kernel]  [k] smp_call_function_many
   - smp_call_function_many
      - 99.22% flush_tlb_page
+   2.51%  [kernel]  [k] rwsem_down_write_failed

It's much more like the 3.15 profile - it's only wasting half the
CPU spinning on the internal spinlock and it's now going fast enough
to be blowing another 10-12% of the CPU time sending tlb flushes to
other CPUs....

One thing I did notice, even with the single-AG-at-a-time run, is
that the system time is *significantly* reduced with this patch,
even though it doesn't change performance.

ie unpatched:

	unpatched	patched
runtime   0m58s		  1m2s
systime   4m55s		  1m1s

So not spinning when there are read holders is a major win even
when there are few threads contending on read/write.

FWIW, the rwsems in the struct xfs_inode are often heavily
read/write contended, so there are lots of IO related workloads that
are going to regress on XFS without this optimisation...

Anyway, consider the patch:

Tested-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Fri, 2014-07-04 at 11:01 +1000, Dave Chinner wrote:
> [re-added lkml]
> 
> On Thu, Jul 03, 2014 at 11:50:20AM -0700, Jason Low wrote:
> > On Wed, Jul 2, 2014 at 7:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> > > This is what the kernel profile looks like on the strided run:
> > >
> > > -  83.06%  [kernel]  [k] osq_lock
> > >    - osq_lock
> > >       - 100.00% rwsem_down_write_failed
> > >          - call_rwsem_down_write_failed
> > >             - 99.55% sys_mprotect
> > >                  tracesys
> > >                  __GI___mprotect
> > > -  12.02%  [kernel]  [k] rwsem_down_write_failed
> > 
> > Hi Dave,
> > 
> > So with no sign of rwsem_spin_on_owner(), yet with such heavy contention in
> > osq_lock, this makes me wonder if it's spending most of its time spinning
> > on !owner while a reader has the lock? (We don't set sem->owner for the readers.)
> > 
> > If that's an issue, maybe the below is worth a test, in which we'll just
> > avoid spinning if rwsem_can_spin_on_owner() finds that there is no owner.
> > If we just had to enter the slowpath yet there is no owner, we'll be conservative
> > and assume readers have the lock.
> 
> That makes it quite a bit faster:
> 
>         XFS_REPAIR Summary    Fri Jul  4 10:39:32 2014
> 
> Phase           Start           End             Duration
> Phase 1:        07/04 10:38:04  07/04 10:38:05  1 second
> Phase 2:        07/04 10:38:05  07/04 10:38:08  3 seconds
> Phase 3:        07/04 10:38:08  07/04 10:39:12  1 minute, 4 seconds
> Phase 4:        07/04 10:39:12  07/04 10:39:21  9 seconds
> Phase 5:        07/04 10:39:21  07/04 10:39:22  1 second
> Phase 6:        07/04 10:39:22  07/04 10:39:30  8 seconds
> Phase 7:        07/04 10:39:30  07/04 10:39:30  
> 
> Total run time: 1 minute, 26 seconds
> done
> 
> real    1m28.504s
> user    1m23.990s
> sys     3m20.132s
> 
> So system time goes down massively, and speed comes up to within 30%
> of the single AG run.  But it's still 2-3000 IOPS down, but it's
> acceptible for the moment.
> 
> FWIW, the kernel profile ifor the multi-AG run now looks like:
> 
>   29.64%  [kernel]  [k] _raw_spin_unlock_irq
>    - _raw_spin_unlock_irq
>       + 35.34% __schedule
>       - 34.15% call_rwsem_down_write_failed
>          + 99.27% sys_mprotect
>       - 30.02% call_rwsem_down_read_failed
>            95.59% __do_page_fault
> -  24.65%  [kernel]  [k] _raw_spin_unlock_irqrestore
>    - _raw_spin_unlock_irqrestore
>       - 69.38% rwsem_wake
>          - call_rwsem_wake
>             - 83.32% sys_mprotect
>             + 15.54% __do_page_fault
>       + 22.55% try_to_wake_up
> +   9.77%  [kernel]  [k] default_send_IPI_mask_sequence_phys
> -   3.21%  [kernel]  [k] smp_call_function_many
>    - smp_call_function_many
>       - 99.22% flush_tlb_page
> +   2.51%  [kernel]  [k] rwsem_down_write_failed
> 
> It's much more like the 3.15 profile - it's only wasting half the
> CPU spinning on the internal spinlock and it's now going fast enough
> to be blowing another 10-12% of the CPU time sending tlb flushes to
> other CPUs....
> 
> One thing I did notice, even with the single-AG-at-a-time run, is
> that the system time is *significantly* reduced with this patch,
> even though it doesn't change performance.
> 
> ie unpatched:
> 
> 	unpatched	patched
> runtime   0m58s		  1m2s
> systime   4m55s		  1m1s
> 
> So not spinning when there are read holders is a major win even
> when there are few threads contending on read/write.
> 
> FWIW, the rwsems in the struct xfs_inode are often heavily
> read/write contended, so there are lots of IO related workloads that
> are going to regress on XFS without this optimisation...
> 
> Anyway, consider the patch:
> 
> Tested-by: Dave Chinner <dchinner@redhat.com>

Hi Dave,

Thanks for testing. I'll update the patch with an actual changelog.

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Thu, 2014-07-03 at 18:46 -0700, Jason Low wrote:
> On Fri, 2014-07-04 at 11:01 +1000, Dave Chinner wrote:

> > FWIW, the rwsems in the struct xfs_inode are often heavily
> > read/write contended, so there are lots of IO related workloads that
> > are going to regress on XFS without this optimisation...
> > 
> > Anyway, consider the patch:
> > 
> > Tested-by: Dave Chinner <dchinner@redhat.com>
> 
> Hi Dave,
> 
> Thanks for testing. I'll update the patch with an actual changelog.

---
Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner

It was found that the rwsem optimistic spinning feature can potentially degrade
performance when there are readers. Perf profiles indicate in some workloads
that significant time can be spent spinning on !owner. This is because we don't
set the lock owner when readers(s) obtain the rwsem.

In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll return
false if there is no lock owner. The rationale is that if we just entered the
slowpath, yet there is no lock owner, then there is a possibility that a reader
has the lock. To be conservative, we'll avoid spinning in these situations.

Dave Chinner found performance benefits with this patch in the xfs_repair
workload, where the total run time went from approximately 4 minutes 24 seconds,
down to approximately 1 minute 26 seconds with the patch.

Tested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jason Low <jason.low2@hp.com>
---
 kernel/locking/rwsem-xadd.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..c40c7d2 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -285,10 +285,10 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
-	bool on_cpu = true;
+	bool on_cpu = false;
 
 	if (need_resched())
-		return 0;
+		return false;
 
 	rcu_read_lock();
 	owner = ACCESS_ONCE(sem->owner);
@@ -297,9 +297,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	rcu_read_unlock();
 
 	/*
-	 * If sem->owner is not set, the rwsem owner may have
-	 * just acquired it and not set the owner yet or the rwsem
-	 * has been released.
+	 * If sem->owner is not set, yet we have just recently entered the
+	 * slowpath, then there is a possibility reader(s) may have the lock.
+	 * To be safe, avoid spinning in these situations.
 	 */
 	return on_cpu;
 }
-- 
1.7.9.5

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Dave Chinner]

On Thu, Jul 03, 2014 at 06:54:50PM -0700, Jason Low wrote:
> On Thu, 2014-07-03 at 18:46 -0700, Jason Low wrote:
> > On Fri, 2014-07-04 at 11:01 +1000, Dave Chinner wrote:
> 
> > > FWIW, the rwsems in the struct xfs_inode are often heavily
> > > read/write contended, so there are lots of IO related workloads that
> > > are going to regress on XFS without this optimisation...
> > > 
> > > Anyway, consider the patch:
> > > 
> > > Tested-by: Dave Chinner <dchinner@redhat.com>
> > 
> > Hi Dave,
> > 
> > Thanks for testing. I'll update the patch with an actual changelog.
> 
> ---
> Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> 
> It was found that the rwsem optimistic spinning feature can potentially degrade
> performance when there are readers. Perf profiles indicate in some workloads
> that significant time can be spent spinning on !owner. This is because we don't
> set the lock owner when readers(s) obtain the rwsem.

I don't think you're being a little shifty with the truth here.
There's no "potentially degrade performance" here - I reported a
massive real world performance regression caused by optimistic
spinning.  That is:

"Commit 4fc828e ("locking/rwsem: Support optimistic spinning")
introduced a major performance regression for workloads such as
xfs_repair which mix read and write locking of the mmap_sem across
many threads. The result was xfs_repair ran 5x slower on 3.16-rc2
than on 3.15 and using 20x more system CPU time."

"Perf profiles indicate....

> In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll return
> false if there is no lock owner. The rationale is that if we just entered the
> slowpath, yet there is no lock owner, then there is a possibility that a reader
> has the lock. To be conservative, we'll avoid spinning in these situations.
> 
> Dave Chinner found performance benefits with this patch in the xfs_repair
> workload, where the total run time went from approximately 4 minutes 24 seconds,
> down to approximately 1 minute 26 seconds with the patch.

Which brought it back to close to the same performance as on 3.15.
This is not a new performance improvement patch - it's a regression
fix and the commit message needs to reflect that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Fri, 2014-07-04 at 16:13 +1000, Dave Chinner wrote:
> On Thu, Jul 03, 2014 at 06:54:50PM -0700, Jason Low wrote:
> > On Thu, 2014-07-03 at 18:46 -0700, Jason Low wrote:
> > > On Fri, 2014-07-04 at 11:01 +1000, Dave Chinner wrote:
> > 
> > > > FWIW, the rwsems in the struct xfs_inode are often heavily
> > > > read/write contended, so there are lots of IO related workloads that
> > > > are going to regress on XFS without this optimisation...
> > > > 
> > > > Anyway, consider the patch:
> > > > 
> > > > Tested-by: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Hi Dave,
> > > 
> > > Thanks for testing. I'll update the patch with an actual changelog.
> > 
> > ---
> > Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> > 
> > It was found that the rwsem optimistic spinning feature can potentially degrade
> > performance when there are readers. Perf profiles indicate in some workloads
> > that significant time can be spent spinning on !owner. This is because we don't
> > set the lock owner when readers(s) obtain the rwsem.
> 
> I don't think you're being a little shifty with the truth here.
> There's no "potentially degrade performance" here - I reported a
> massive real world performance regression caused by optimistic
> spinning.

Sure, though I mainly used the word "potentially" since there can be
other workloads out there where spinning even when readers have the lock
is a positive thing.

And agreed that the changelog can be modified to try reflecting more on
it being a "regression fix" then a "new performance" addition.

So how about the following?

---
Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner

Commit 4fc828e24cd9 ("locking/rwsem: Support optimistic spinning")
introduced a major performance regression for workloads such as
xfs_repair which mix read and write locking of the mmap_sem across
many threads. The result was xfs_repair ran 5x slower on 3.16-rc2
than on 3.15 and using 20x more system CPU time.

Perf profiles indicate in some workloads that significant time can
be spent spinning on !owner. This is because we don't set the lock
owner when readers(s) obtain the rwsem.

In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll
return false if there is no lock owner. The rationale is that if we
just entered the slowpath, yet there is no lock owner, then there is
a possibility that a reader has the lock. To be conservative, we'll
avoid spinning in these situations.

This patch reduced the total run time of the xfs_repair workload from
about 4 minutes 24 seconds down to approximately 1 minute 26 seconds,
back to close to the same performance as on 3.15.

Tested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jason Low <jason.low2@hp.com>
---
 kernel/locking/rwsem-xadd.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..c40c7d2 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -285,10 +285,10 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
-	bool on_cpu = true;
+	bool on_cpu = false;
 
 	if (need_resched())
-		return 0;
+		return false;
 
 	rcu_read_lock();
 	owner = ACCESS_ONCE(sem->owner);
@@ -297,9 +297,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	rcu_read_unlock();
 
 	/*
-	 * If sem->owner is not set, the rwsem owner may have
-	 * just acquired it and not set the owner yet or the rwsem
-	 * has been released.
+	 * If sem->owner is not set, yet we have just recently entered the
+	 * slowpath, then there is a possibility reader(s) may have the lock.
+	 * To be safe, avoid spinning in these situations.
 	 */
 	return on_cpu;
 }
-- 
1.7.9.5

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Dave Chinner]

On Fri, Jul 04, 2014 at 12:06:19AM -0700, Jason Low wrote:
> On Fri, 2014-07-04 at 16:13 +1000, Dave Chinner wrote:
> > On Thu, Jul 03, 2014 at 06:54:50PM -0700, Jason Low wrote:
> > > On Thu, 2014-07-03 at 18:46 -0700, Jason Low wrote:
> > > > On Fri, 2014-07-04 at 11:01 +1000, Dave Chinner wrote:
> > > 
> > > > > FWIW, the rwsems in the struct xfs_inode are often heavily
> > > > > read/write contended, so there are lots of IO related workloads that
> > > > > are going to regress on XFS without this optimisation...
> > > > > 
> > > > > Anyway, consider the patch:
> > > > > 
> > > > > Tested-by: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Hi Dave,
> > > > 
> > > > Thanks for testing. I'll update the patch with an actual changelog.
> > > 
> > > ---
> > > Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> > > 
> > > It was found that the rwsem optimistic spinning feature can potentially degrade
> > > performance when there are readers. Perf profiles indicate in some workloads
> > > that significant time can be spent spinning on !owner. This is because we don't
> > > set the lock owner when readers(s) obtain the rwsem.
> > 
> > I don't think you're being a little shifty with the truth here.
> > There's no "potentially degrade performance" here - I reported a
> > massive real world performance regression caused by optimistic
> > spinning.
> 
> Sure, though I mainly used the word "potentially" since there can be
> other workloads out there where spinning even when readers have the lock
> is a positive thing.
> 
> And agreed that the changelog can be modified to try reflecting more on
> it being a "regression fix" then a "new performance" addition.
> 
> So how about the following?

Looks good. Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Davidlohr Bueso]

On Fri, 2014-07-04 at 00:06 -0700, Jason Low 
> Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner

Could we change the subject to something more descriptive, ie:

rwsem: Allow conservative optimistic spinning for reader/writer paths

Thanks,
Davidlohr

> Commit 4fc828e24cd9 ("locking/rwsem: Support optimistic spinning")
> introduced a major performance regression for workloads such as
> xfs_repair which mix read and write locking of the mmap_sem across
> many threads. The result was xfs_repair ran 5x slower on 3.16-rc2
> than on 3.15 and using 20x more system CPU time.
> 
> Perf profiles indicate in some workloads that significant time can
> be spent spinning on !owner. This is because we don't set the lock
> owner when readers(s) obtain the rwsem.
> 
> In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll
> return false if there is no lock owner. The rationale is that if we
> just entered the slowpath, yet there is no lock owner, then there is
> a possibility that a reader has the lock. To be conservative, we'll
> avoid spinning in these situations.
> 
> This patch reduced the total run time of the xfs_repair workload from
> about 4 minutes 24 seconds down to approximately 1 minute 26 seconds,
> back to close to the same performance as on 3.15.
> 
> Tested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jason Low <jason.low2@hp.com>
> ---
>  kernel/locking/rwsem-xadd.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index dacc321..c40c7d2 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -285,10 +285,10 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
>  static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>  {
>  	struct task_struct *owner;
> -	bool on_cpu = true;
> +	bool on_cpu = false;
>  
>  	if (need_resched())
> -		return 0;
> +		return false;
>  
>  	rcu_read_lock();
>  	owner = ACCESS_ONCE(sem->owner);
> @@ -297,9 +297,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>  	rcu_read_unlock();
>  
>  	/*
> -	 * If sem->owner is not set, the rwsem owner may have
> -	 * just acquired it and not set the owner yet or the rwsem
> -	 * has been released.
> +	 * If sem->owner is not set, yet we have just recently entered the
> +	 * slowpath, then there is a possibility reader(s) may have the lock.
> +	 * To be safe, avoid spinning in these situations.
>  	 */
>  	return on_cpu;
>  }

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Fri, 2014-07-04 at 01:53 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-07-04 at 00:06 -0700, Jason Low 
> > Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> 
> Could we change the subject to something more descriptive, ie:
> 
> rwsem: Allow conservative optimistic spinning for reader/writer paths

Sure, a Subject like this would better explain what is the effect of the
patch.

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Thu, Jul 03, 2014 at 06:54:50PM -0700, Jason Low wrote:
> Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> 
> It was found that the rwsem optimistic spinning feature can potentially degrade
> performance when there are readers. Perf profiles indicate in some workloads
> that significant time can be spent spinning on !owner. This is because we don't
> set the lock owner when readers(s) obtain the rwsem.
> 
> In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll return
> false if there is no lock owner. The rationale is that if we just entered the
> slowpath, yet there is no lock owner, then there is a possibility that a reader
> has the lock. To be conservative, we'll avoid spinning in these situations.
> 
> Dave Chinner found performance benefits with this patch in the xfs_repair
> workload, where the total run time went from approximately 4 minutes 24 seconds,
> down to approximately 1 minute 26 seconds with the patch.
> 
> Tested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jason Low <jason.low2@hp.com>

Davidlohr, you'll be running this through your AIM and other benchmarks,
I suppose?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Davidlohr Bueso]

On Fri, 2014-07-04 at 09:52 +0200, Peter Zijlstra wrote:
> On Thu, Jul 03, 2014 at 06:54:50PM -0700, Jason Low wrote:
> > Subject: [PATCH] rwsem: In rwsem_can_spin_on_owner(), return false if no owner
> > 
> > It was found that the rwsem optimistic spinning feature can potentially degrade
> > performance when there are readers. Perf profiles indicate in some workloads
> > that significant time can be spent spinning on !owner. This is because we don't
> > set the lock owner when readers(s) obtain the rwsem.
> > 
> > In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll return
> > false if there is no lock owner. The rationale is that if we just entered the
> > slowpath, yet there is no lock owner, then there is a possibility that a reader
> > has the lock. To be conservative, we'll avoid spinning in these situations.
> > 
> > Dave Chinner found performance benefits with this patch in the xfs_repair
> > workload, where the total run time went from approximately 4 minutes 24 seconds,
> > down to approximately 1 minute 26 seconds with the patch.
> > 
> > Tested-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Jason Low <jason.low2@hp.com>
> 
> Davidlohr, you'll be running this through your AIM and other benchmarks,
> I suppose?

I ran it through aim7, and as I suspected we take a performance hit with
'custom' ~-14% throughput for > 300 users (which is still overall quite
higher than rwsems without opt spinning, at around ~+45%), and we
actually improve a bit (~+15%) in 'disk' with >1000 users -- which
afaict resembles Dave's workload a bit. So all in all I'm quite happy
with this patch.

Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Fri, 2014-07-04 at 01:40 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-07-04 at 09:52 +0200, Peter Zijlstra wrote:
> > Davidlohr, you'll be running this through your AIM and other benchmarks,
> > I suppose?
> 
> I ran it through aim7, and as I suspected we take a performance hit with
> 'custom' ~-14% throughput for > 300 users (which is still overall quite
> higher than rwsems without opt spinning, at around ~+45%), and we
> actually improve a bit (~+15%) in 'disk' with >1000 users -- which
> afaict resembles Dave's workload a bit. So all in all I'm quite happy
> with this patch.

Here is the patch with the updates to the changelog.

---
Subject: [PATCH] rwsem: Allow conservative optimistic spinning when readers have lock

Commit 4fc828e24cd9 ("locking/rwsem: Support optimistic spinning")
introduced a major performance regression for workloads such as
xfs_repair which mix read and write locking of the mmap_sem across
many threads. The result was xfs_repair ran 5x slower on 3.16-rc2
than on 3.15 and using 20x more system CPU time.

Perf profiles indicate in some workloads that significant time can
be spent spinning on !owner. This is because we don't set the lock
owner when readers(s) obtain the rwsem.

In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll
return false if there is no lock owner. The rationale is that if we
just entered the slowpath, yet there is no lock owner, then there is
a possibility that a reader has the lock. To be conservative, we'll
avoid spinning in these situations.

This patch reduced the total run time of the xfs_repair workload from
about 4 minutes 24 seconds down to approximately 1 minute 26 seconds,
back to close to the same performance as on 3.15.

Retesting of AIM7, which were some of the workloads used to test the
original optimistic spinning code, confirmed that we still get big
performance gains with optimistic spinning, even with this additional
regression fix. Davidlohr found that while the 'custom' workload took
a performance hit of ~-14% to throughput for >300 users with this
additional patch, the overall gain with optimistic spinning is
still ~+45%. The 'disk' workload even improved by ~+15% at >1000 users.

Tested-by: Dave Chinner <dchinner@redhat.com>
Acked-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Jason Low <jason.low2@hp.com>
---
 kernel/locking/rwsem-xadd.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..c40c7d2 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -285,10 +285,10 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
-	bool on_cpu = true;
+	bool on_cpu = false;
 
 	if (need_resched())
-		return 0;
+		return false;
 
 	rcu_read_lock();
 	owner = ACCESS_ONCE(sem->owner);
@@ -297,9 +297,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	rcu_read_unlock();
 
 	/*
-	 * If sem->owner is not set, the rwsem owner may have
-	 * just acquired it and not set the owner yet or the rwsem
-	 * has been released.
+	 * If sem->owner is not set, yet we have just recently entered the
+	 * slowpath, then there is a possibility reader(s) may have the lock.
+	 * To be safe, avoid spinning in these situations.
 	 */
 	return on_cpu;
 }
-- 
1.7.9.5

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Peter Zijlstra]

On Mon, Jul 14, 2014 at 05:37:13PM +0800, Xie Miao wrote:
> Hi, Jason
> 
> Could you re-sent this patch? Because it seems it is ignored.

Its not ignored; its not merged because of unrelated stability issues.

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Jason Low]

On Mon, 2014-07-14 at 11:55 +0200, Peter Zijlstra wrote:
> On Mon, Jul 14, 2014 at 05:37:13PM +0800, Xie Miao wrote:
> > Hi, Jason
> > 
> > Could you re-sent this patch? Because it seems it is ignored.
> 
> Its not ignored;

Right, I also noticed the patch in the git peterz/queue tree :)

Re: [regression, 3.16-rc] rwsem: optimistic spinning causing performance degradation

[Dave Chinner]

On Mon, Jul 14, 2014 at 11:55:39AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 14, 2014 at 05:37:13PM +0800, Xie Miao wrote:
> > Hi, Jason
> > 
> > Could you re-sent this patch? Because it seems it is ignored.
> 
> Its not ignored; its not merged because of unrelated stability issues.

Is it going to be merged? If not, can you please revert the
optimistic spinning patch that caused the regression?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[tip:locking/urgent] locking/rwsem: Allow conservative optimistic spinning when readers have lock

[tip-bot for Jason Low]

Commit-ID:  37e9562453b813d2ea527bd9531fef2c3c592847
Gitweb:     http://git.kernel.org/tip/37e9562453b813d2ea527bd9531fef2c3c592847
Author:     Jason Low <jason.low2@hp.com>
AuthorDate: Fri, 4 Jul 2014 20:49:32 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 Jul 2014 13:28:02 +0200

locking/rwsem: Allow conservative optimistic spinning when readers have lock

Commit 4fc828e24cd9 ("locking/rwsem: Support optimistic spinning")
introduced a major performance regression for workloads such as
xfs_repair which mix read and write locking of the mmap_sem across
many threads. The result was xfs_repair ran 5x slower on 3.16-rc2
than on 3.15 and using 20x more system CPU time.

Perf profiles indicate in some workloads that significant time can
be spent spinning on !owner. This is because we don't set the lock
owner when readers(s) obtain the rwsem.

In this patch, we'll modify rwsem_can_spin_on_owner() such that we'll
return false if there is no lock owner. The rationale is that if we
just entered the slowpath, yet there is no lock owner, then there is
a possibility that a reader has the lock. To be conservative, we'll
avoid spinning in these situations.

This patch reduced the total run time of the xfs_repair workload from
about 4 minutes 24 seconds down to approximately 1 minute 26 seconds,
back to close to the same performance as on 3.15.

Retesting of AIM7, which were some of the workloads used to test the
original optimistic spinning code, confirmed that we still get big
performance gains with optimistic spinning, even with this additional
regression fix. Davidlohr found that while the 'custom' workload took
a performance hit of ~-14% to throughput for >300 users with this
additional patch, the overall gain with optimistic spinning is
still ~+45%. The 'disk' workload even improved by ~+15% at >1000 users.

Tested-by: Dave Chinner <dchinner@redhat.com>
Acked-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Jason Low <jason.low2@hp.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1404532172.2572.30.camel@j-VirtualBox
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/rwsem-xadd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..c40c7d2 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -285,10 +285,10 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 {
 	struct task_struct *owner;
-	bool on_cpu = true;
+	bool on_cpu = false;
 
 	if (need_resched())
-		return 0;
+		return false;
 
 	rcu_read_lock();
 	owner = ACCESS_ONCE(sem->owner);
@@ -297,9 +297,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	rcu_read_unlock();
 
 	/*
-	 * If sem->owner is not set, the rwsem owner may have
-	 * just acquired it and not set the owner yet or the rwsem
-	 * has been released.
+	 * If sem->owner is not set, yet we have just recently entered the
+	 * slowpath, then there is a possibility reader(s) may have the lock.
+	 * To be safe, avoid spinning in these situations.
 	 */
 	return on_cpu;
 }