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


Groups > linux.kernel > #1218680 > unrolled thread

[4.2, Regression] Queued spinlocks cause major XFS performance regression

Started byDave Chinner <david@fromorbit.com>
First post2015-09-04 07:50 +0200
Last post2015-09-10 04:20 +0200
Articles 20 on this page of 29 — 7 participants

Back to article view | Back to linux.kernel


Contents

  [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 07:50 +0200
    Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 08:40 +0200
      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 09:20 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Juergen Gross <jgross@suse.com> - 2015-09-04 09:40 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 10:00 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 10:40 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 17:10 +0200
            Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:20 +0200
              Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:30 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-07 01:40 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Davidlohr Bueso <dave@stgolabs.net> - 2015-09-07 02:10 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-07 09:00 +0200
                    Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-07 22:50 +0200
                      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Davidlohr Bueso <dave@stgolabs.net> - 2015-09-08 08:40 +0200
                      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-08 12:10 +0200
                        Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-08 19:50 +0200
                [tip:locking/core] locking/qspinlock/x86:   Fix performance regression under unaccelerated VMs tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2015-09-13 13:00 +0200
              Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 17:30 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:40 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 18:00 +0200
                    Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Waiman Long <waiman.long@hpe.com> - 2015-09-10 04:20 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 18:00 +0200
                    Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-05 19:50 +0200
      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 09:40 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 13:40 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-05 00:10 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-07 01:50 +0200
            Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Waiman Long <waiman.long@hpe.com> - 2015-09-10 04:10 +0200
    Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Waiman Long <waiman.long@hpe.com> - 2015-09-10 04:20 +0200

Page 1 of 2  [1] 2  Next page →


#1218680 — [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromDave Chinner <david@fromorbit.com>
Date2015-09-04 07:50 +0200
Subject[4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q4S2S-1oE-5@gated-at.bofh.it>
Hi Waiman,

For the first time in months I just turned of spinlock debugging on
my performance test machine and I just got an unpleasant surprise on
my standard inode allocation and reclaim test.  I've described this
test to you before, because it's found regressions in your previous
lock scaling changes:

http://permalink.gmane.org/gmane.linux.kernel/1768786

This time it is the fsmark run that I use to populate the filesystem
that is demonstrating a locking regression. I'll asked you before
if you could add this test to your lock scaling regression test
suite; please do it this time.

Now, the regression.  With spinlock debugging turned on, the
performance of my usual XFS inode allocation benchmark using fsmark
reports performance like this:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     312594.0          9944159
     0      3200000            0     295668.6         10399679
     0      4800000            0     279026.1         11397617
.....

This has been pretty stable for several releases - it varies +/- a
few percent, but it's pretty much been like this since about 3.2
when CONFIG_XFS_DEBUG=n, with or without basic spinlock debugging.

When I turned spinlock debugging off on 4.2 to get some perf numbers
a request from Linus, I got this:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     114143.9          9597599
     0      3200000            0      95486.9          9460413
     0      4800000            0      93918.2          9784699
....

All 16 CPUs were pegged at 100% cpu usage. I took a quick look at
perf:

  67.32%  [kernel]       [k] queued_spin_lock_slowpath
   5.17%  [kernel]       [k] xfs_log_commit_cil
   2.47%  [kernel]       [k] _xfs_buf_find
   1.37%  [kernel]       [k] _raw_spin_lock
   ....

And then a quick call graph sample to find the lock:

   37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
   - queued_spin_lock_slowpath
      - 99.98% _raw_spin_lock
         - 89.16% xfs_log_commit_cil
            - __xfs_trans_commit
               - 98.48% xfs_trans_commit
                    xfs_create
                    xfs_generic_create
                    xfs_vn_mknod
                    xfs_vn_create
                    vfs_create
                    path_openat
                    do_filp_open
                    do_sys_open
                    sys_open
                    entry_SYSCALL_64_fastpath
                  + __GI___libc_open
               + 1.52% __xfs_trans_roll

This shows that we have catastrophic spinlock contention in the
transaction commit path. The cil->xc_cil_lock spin lock as it's the
only spinlock in that path. And while it's the hot lock in the
commit path, turning spinlock debugging back on (and no other
changes) shows that it shouldn't be contended:

   8.92%  [kernel]  [k] _xfs_buf_find
   5.51%  [kernel]  [k] xfs_dir2_node_addname
   3.49%  [kernel]  [k] xfs_dir3_free_hdr_from_disk
   3.45%  [kernel]  [k] do_raw_spin_lock
   3.06%  [kernel]  [k] kmem_cache_alloc
   2.99%  [kernel]  [k] __memcpy
   2.97%  [kernel]  [k] xfs_log_commit_cil
   .....

And the call graph:

    4.52%     0.24%  [kernel]            [k] _raw_spin_lock
   - _raw_spin_lock
      + 43.63% xfs_log_commit_cil
      + 9.76% inode_sb_list_add
      + 8.57% list_lru_add
      + 6.37% _xfs_buf_find
      + 6.08% d_alloc
      + 4.18% dput
      + 4.08% __d_instantiate
...

IOWs, about 2% of the cpu usage is lock traffic through
cil->xc_cil_lock, and that's 3x the locking rate where the queued
spinlock becomes catastrophically contention bound.

To confirm that this is indeed caused by the queued spinlocks, I
removed the the spinlock debugging and did this to arch/x86/Kconfig:

-       select ARCH_USE_QUEUED_SPINLOCK

And the results are:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     329310.4          9727415
     0      3200000            0     305421.4         10421358
     0      4800000            0     294593.3         11007112
     0      6400000            0     283863.7         12557190
....

So the problem is definitely the queued spinlock...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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]


#1218686 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-09-04 08:40 +0200
SubjectRe: [4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q4SPg-2xi-3@gated-at.bofh.it>
In reply to#1218680
On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> When I turned spinlock debugging off on 4.2 to get some perf numbers
> a request from Linus, I got this:

[ ugly numbers deleted ]

> And then a quick call graph sample to find the lock:
>
>    37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
>    - queued_spin_lock_slowpath
>       - 99.98% _raw_spin_lock
>          - 89.16% xfs_log_commit_cil
[ snip ]
>
> This shows that we have catastrophic spinlock contention in the
> transaction commit path. The cil->xc_cil_lock spin lock as it's the
> only spinlock in that path. And while it's the hot lock in the
> commit path, turning spinlock debugging back on (and no other
> changes) shows that it shouldn't be contended:
>
>    8.92%  [kernel]  [k] _xfs_buf_find
[ snip ]

So you basically have almost no spinlock overhead at all even when
debugging is on.

That's unusual, as usually the debug code makes the contention much much worse.

> To confirm that this is indeed caused by the queued spinlocks, I
> removed the the spinlock debugging and did this to arch/x86/Kconfig:
>
> -       select ARCH_USE_QUEUED_SPINLOCK
>
> And the results are:

Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
but numbers talk, BS walks.

If I read things right, the actual spinlock is the "cil->xc_cil_lock"
that is taken in xlog_cil_insert_items(), and it justr shows up in
xfs_log_commit_cil() in the call graph due to inlining. Correct?

There doesn't seem to be anything even remotely strange going on in that area.

Is this a PARAVIRT configuration? There were issues with PV
interaction at some point. If it is PV, and you don't actually use PV,
can you test with PV support disabled?

Also, if you look at the instruction-level profile for
queued_spin_lock_slowpath itself, does anything stand out? For
example, I note that the for-loop with the atomic_cmpxchg() call in it
doesn't ever do a cpu_relax(). It doesn't look like that should
normally loop, but obviously that function also shouldn't normally use
2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
stands out as "it's spending 99% of the time in _that_ particular
part, and it gives some clue what goes wrong.

Ugh. Added Ingo to the cc. I *hate* locking problems, to the point
where I think we need to seriously consider undoing the queued
spinlocks if they have these kinds of odd issues..

Anybody have any ideas?

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


#1218698

FromDave Chinner <david@fromorbit.com>
Date2015-09-04 09:20 +0200
Message-ID<q4TrX-3vn-9@gated-at.bofh.it>
In reply to#1218686
On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > When I turned spinlock debugging off on 4.2 to get some perf numbers
> > a request from Linus, I got this:
> 
> [ ugly numbers deleted ]
> 
> > And then a quick call graph sample to find the lock:
> >
> >    37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
> >    - queued_spin_lock_slowpath
> >       - 99.98% _raw_spin_lock
> >          - 89.16% xfs_log_commit_cil
> [ snip ]
> >
> > This shows that we have catastrophic spinlock contention in the
> > transaction commit path. The cil->xc_cil_lock spin lock as it's the
> > only spinlock in that path. And while it's the hot lock in the
> > commit path, turning spinlock debugging back on (and no other
> > changes) shows that it shouldn't be contended:
> >
> >    8.92%  [kernel]  [k] _xfs_buf_find
> [ snip ]
> 
> So you basically have almost no spinlock overhead at all even when
> debugging is on.

*nod*

> That's unusual, as usually the debug code makes the contention much much worse.

Right. The debug behaviour is completely unchanged, that's why I
didn't notice this earlier. And it's not until I scale this workload
to >32p that is tend to see and significant level of contention on
the cil->xc_cil_lock when the basic spin lock debugging is enabled.

> > To confirm that this is indeed caused by the queued spinlocks, I
> > removed the the spinlock debugging and did this to arch/x86/Kconfig:
> >
> > -       select ARCH_USE_QUEUED_SPINLOCK
> >
> > And the results are:
> 
> Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
> but numbers talk, BS walks.
> 
> If I read things right, the actual spinlock is the "cil->xc_cil_lock"
> that is taken in xlog_cil_insert_items(), and it justr shows up in
> xfs_log_commit_cil() in the call graph due to inlining. Correct?

Yup, that's how I read it, too. 

> There doesn't seem to be anything even remotely strange going on in that area.
> 
> Is this a PARAVIRT configuration? There were issues with PV
> interaction at some point. If it is PV, and you don't actually use PV,
> can you test with PV support disabled?

$ grep PARAVIRT .config
CONFIG_PARAVIRT=y
# CONFIG_PARAVIRT_DEBUG is not set
# CONFIG_PARAVIRT_SPINLOCKS is not set
CONFIG_PARAVIRT_TIME_ACCOUNTING=y
CONFIG_PARAVIRT_CLOCK=y
$

I'll retest with CONFIG_PARAVIRT=n....

> Also, if you look at the instruction-level profile for
> queued_spin_lock_slowpath itself, does anything stand out? For
> example, I note that the for-loop with the atomic_cmpxchg() call in it
> doesn't ever do a cpu_relax(). It doesn't look like that should
> normally loop, but obviously that function also shouldn't normally use
> 2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
> stands out as "it's spending 99% of the time in _that_ particular
> part, and it gives some clue what goes wrong.

I'll have a look when the current tests on that machine have
finished running.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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]


#1218704

FromJuergen Gross <jgross@suse.com>
Date2015-09-04 09:40 +0200
Message-ID<q4TLj-3RF-1@gated-at.bofh.it>
In reply to#1218698
On 09/04/2015 09:11 AM, Dave Chinner wrote:
> On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
>> On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
>>>
>>> When I turned spinlock debugging off on 4.2 to get some perf numbers
>>> a request from Linus, I got this:
>>
>> [ ugly numbers deleted ]
>>
>>> And then a quick call graph sample to find the lock:
>>>
>>>     37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
>>>     - queued_spin_lock_slowpath
>>>        - 99.98% _raw_spin_lock
>>>           - 89.16% xfs_log_commit_cil
>> [ snip ]
>>>
>>> This shows that we have catastrophic spinlock contention in the
>>> transaction commit path. The cil->xc_cil_lock spin lock as it's the
>>> only spinlock in that path. And while it's the hot lock in the
>>> commit path, turning spinlock debugging back on (and no other
>>> changes) shows that it shouldn't be contended:
>>>
>>>     8.92%  [kernel]  [k] _xfs_buf_find
>> [ snip ]
>>
>> So you basically have almost no spinlock overhead at all even when
>> debugging is on.
>
> *nod*
>
>> That's unusual, as usually the debug code makes the contention much much worse.
>
> Right. The debug behaviour is completely unchanged, that's why I
> didn't notice this earlier. And it's not until I scale this workload
> to >32p that is tend to see and significant level of contention on
> the cil->xc_cil_lock when the basic spin lock debugging is enabled.
>
>>> To confirm that this is indeed caused by the queued spinlocks, I
>>> removed the the spinlock debugging and did this to arch/x86/Kconfig:
>>>
>>> -       select ARCH_USE_QUEUED_SPINLOCK
>>>
>>> And the results are:
>>
>> Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
>> but numbers talk, BS walks.
>>
>> If I read things right, the actual spinlock is the "cil->xc_cil_lock"
>> that is taken in xlog_cil_insert_items(), and it justr shows up in
>> xfs_log_commit_cil() in the call graph due to inlining. Correct?
>
> Yup, that's how I read it, too.
>
>> There doesn't seem to be anything even remotely strange going on in that area.
>>
>> Is this a PARAVIRT configuration? There were issues with PV
>> interaction at some point. If it is PV, and you don't actually use PV,
>> can you test with PV support disabled?
>
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $
>
> I'll retest with CONFIG_PARAVIRT=n....

Shouldn't matter at all. CONFIG_PARAVIRT_SPINLOCKS isn't set, so the
locks aren't para-virtualized.


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


#1218715

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-04 10:00 +0200
Message-ID<q4U4G-4eq-13@gated-at.bofh.it>
In reply to#1218698
On Fri, Sep 04, 2015 at 05:11:43PM +1000, Dave Chinner wrote:
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $

Curious, with PARAVIRT_SPINLOCKS=n we _should_ end up in
virt_queued_spin_lock() which is a simple test-and-set lock, much like
the lock debugging code should end up being.


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


#1218762

FromDave Chinner <david@fromorbit.com>
Date2015-09-04 10:40 +0200
Message-ID<q4UHo-5cD-15@gated-at.bofh.it>
In reply to#1218698
On Fri, Sep 04, 2015 at 05:11:43PM +1000, Dave Chinner wrote:
> On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
> > There doesn't seem to be anything even remotely strange going on in that area.
> > 
> > Is this a PARAVIRT configuration? There were issues with PV
> > interaction at some point. If it is PV, and you don't actually use PV,
> > can you test with PV support disabled?
> 
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $
> 
> I'll retest with CONFIG_PARAVIRT=n....

$ grep PARAVIRT .config
# CONFIG_PARAVIRT is not set
$


FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     123407.7          9202289
     0      3200000            0      97271.9          9187905
     0      4800000            0     101010.3         11246527
....

So, no, that doesn't affect the queued spinlock performance at all.

> > Also, if you look at the instruction-level profile for
> > queued_spin_lock_slowpath itself, does anything stand out? For
> > example, I note that the for-loop with the atomic_cmpxchg() call in it
> > doesn't ever do a cpu_relax(). It doesn't look like that should
> > normally loop, but obviously that function also shouldn't normally use
> > 2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
> > stands out as "it's spending 99% of the time in _that_ particular
> > part, and it gives some clue what goes wrong.
> 
> I'll have a look when the current tests on that machine have
> finished running.

      ¿     Disassembly of section load2:
       ¿
       ¿     ffffffff810e0f30 <load2+0xe0f30>:
  0.00 ¿       nop
       ¿       push   %rbp
  0.00 ¿       mov    %rsp,%rbp
  0.00 ¿       xchg   %ax,%ax
       ¿       xor    %eax,%eax
  0.00 ¿       mov    $0x1,%edx
       ¿       lock   cmpxchg %edx,(%rdi)
  0.33 ¿       xor    %ecx,%ecx
       ¿       test   %eax,%eax
       ¿     ¿ je     28
  0.02 ¿ 1c:   pause
  4.45 ¿       mov    %ecx,%eax
  0.00 ¿       lock   cmpxchg %edx,(%rdi)
 95.18 ¿       test   %eax,%eax
       ¿     ¿ jne    1c
  0.01 ¿ 28:   pop    %rbp
  0.01 ¿     ¿ retq

.....

It looks like it's spending all it's time looping around the cmpxchg.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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]


#1219103 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-09-04 17:10 +0200
SubjectRe: [4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q50MO-5FD-9@gated-at.bofh.it>
In reply to#1218762
On Fri, Sep 4, 2015 at 1:29 AM, Dave Chinner <david@fromorbit.com> wrote:
>
...
>   0.02  1c:   pause
>   4.45        mov    %ecx,%eax
>   0.00        lock   cmpxchg %edx,(%rdi)
>  95.18        test   %eax,%eax
>                jne    1c
...
> It looks like it's spending all it's time looping around the cmpxchg.

That code sequence doesn't look sensible. Busy-looping on a cmpxchg is
insane - if you are busy-looping, you should always make sure the
inner tight loop is done while waiting for the value.

It seems to come from virt_queued_spin_lock(), and that just looks
like completely bogus crap.

PeterZ, this is your magical hypervisor thing, and I get the feeling
that that explains why Dave sees nasty performance: most people have
tested either on raw hardware or using the actual paravirtualized
ones, but this is the case for "we're running with a hypervisor, but
not paravirtualized".

So virt_queued_spin_lock() for the hypervisor case looks completely
buggered to me for several reasons:

 - it doesn't actually ever use any queueing, since it always returns true

   so the "queued spinlocks" in this case aren't actually queued, and
they aren't even ticket-locks, they are just plain 0/1 values if I
read things right.

 - the busy-loop to set the queued spinlock uses that cmpxchg in a
tight loop, which kills any memory subsystem. That's unacceptable.

So at the very *minimum*, that second issue should be fixed, and the
loop in virt_queued_spin_lock() should look something like

    do {
        while (READ_ONCE(lock->val) != 0)
            cpu_relax();
    } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);

which at least has a chance in hell of behaving well on the bus and in
a HT environment.

But I suspect that it would be even better for Dave to just disable
the whole thing, and see how the queued locks actually work. Dave, can
you turn that virt_queued_spin_lock() into just "return false"? In
fact, I would almost _insist_ we do this when CONFIG_PARAVIRT_SPINLOCK
isn't set, isn't that what our old ticket-spinlocks did? They didn't
screw up and degrade to a test-and-set lock just because they saw a
hypervisor - that only happened when things were paravirt-aware. No?

Dave, if you have the energy, try it both ways. But the code as-is for
"I'm running in a hypervisor" looks just terminally broken. People who
didn't run in hypervisors just never saw the breakage.

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


#1219109

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-04 17:20 +0200
Message-ID<q50Wu-5QK-11@gated-at.bofh.it>
In reply to#1219103
On Fri, Sep 04, 2015 at 08:05:16AM -0700, Linus Torvalds wrote:
> So at the very *minimum*, that second issue should be fixed, and the
> loop in virt_queued_spin_lock() should look something like
> 
>     do {
>         while (READ_ONCE(lock->val) != 0)
>             cpu_relax();
>     } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> 
> which at least has a chance in hell of behaving well on the bus and in
> a HT environment.

True.

> But I suspect that it would be even better for Dave to just disable
> the whole thing, and see how the queued locks actually work. Dave, can
> you turn that virt_queued_spin_lock() into just "return false"? In
> fact, I would almost _insist_ we do this when CONFIG_PARAVIRT_SPINLOCK
> isn't set, isn't that what our old ticket-spinlocks did? They didn't
> screw up and degrade to a test-and-set lock just because they saw a
> hypervisor - that only happened when things were paravirt-aware. No?

The reason we chose to revert to a test-and-set is because regular fair
locks, like the ticket and the queue thing, have horrible behaviour
under vcpu preemption.

> Dave, if you have the energy, try it both ways. But the code as-is for
> "I'm running in a hypervisor" looks just terminally broken. People who
> didn't run in hypervisors just never saw the breakage.

He did, it mostly restores performance, but was quite erratic. Lock
holder preemption problems get much worse with strict queueing. So even
though he's typically not overloaded, any vcpu preemption can ripple
through and create noise.


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


#1219116

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-04 17:30 +0200
Message-ID<q516c-61Z-29@gated-at.bofh.it>
In reply to#1219109
On Fri, Sep 04, 2015 at 05:14:27PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 04, 2015 at 08:05:16AM -0700, Linus Torvalds wrote:
> > So at the very *minimum*, that second issue should be fixed, and the
> > loop in virt_queued_spin_lock() should look something like
> > 
> >     do {
> >         while (READ_ONCE(lock->val) != 0)
> >             cpu_relax();
> >     } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> > 
> > which at least has a chance in hell of behaving well on the bus and in
> > a HT environment.
> 
> True.

Something like so...

---
Subject: locking: Fix virt test-and-set lock implementation

Dave ran into horrible performance on a VM without PARAVIRT_SPINLOCKS
set and Linus noted that the test-and-set implementation was retarded.

One should spin on the variable with a load, not a rmw.

While there, remove the queued from the name, as the lock isn't queued
at all, but a simple test-and-set.

Reported-by: Dave Chinner <david@fromorbit.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/qspinlock.h | 16 ++++++++++++----
 include/asm-generic/qspinlock.h  |  4 ++--
 kernel/locking/qspinlock.c       |  2 +-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 9d51fae1cba3..8dde3bdc4a05 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -39,15 +39,23 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 #endif
 
-#define virt_queued_spin_lock virt_queued_spin_lock
+#define virt_spin_lock virt_spin_lock
 
-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
+static inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
 		return false;
 
-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
-		cpu_relax();
+	/*
+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
+	 * back to a Test-and-Set spinlock, because fair locks have
+	 * horrible lock 'holder' preemption issues.
+	 */
+
+	do {
+		while (atomic_read(&lock->val) != 0)
+			cpu_relax();
+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
 
 	return true;
 }
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index 83bfb87f5bf1..e2aadbc7151f 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -111,8 +111,8 @@ static inline void queued_spin_unlock_wait(struct qspinlock *lock)
 		cpu_relax();
 }
 
-#ifndef virt_queued_spin_lock
-static __always_inline bool virt_queued_spin_lock(struct qspinlock *lock)
+#ifndef virt_spin_lock
+static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	return false;
 }
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 337c8818541d..87e9ce6a63c5 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -289,7 +289,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 	if (pv_enabled())
 		goto queue;
 
-	if (virt_queued_spin_lock(lock))
+	if (virt_spin_lock(lock))
 		return;
 
 	/*
--
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]


#1219874

FromDave Chinner <david@fromorbit.com>
Date2015-09-07 01:40 +0200
Message-ID<q5RHr-54u-5@gated-at.bofh.it>
In reply to#1219116
On Fri, Sep 04, 2015 at 05:25:23PM +0200, Peter Zijlstra wrote:
> Something like so...
> 
> ---
> Subject: locking: Fix virt test-and-set lock implementation
> 
> Dave ran into horrible performance on a VM without PARAVIRT_SPINLOCKS
> set and Linus noted that the test-and-set implementation was retarded.
> 
> One should spin on the variable with a load, not a rmw.
> 
> While there, remove the queued from the name, as the lock isn't queued
> at all, but a simple test-and-set.
> 
> Reported-by: Dave Chinner <david@fromorbit.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     319511.8          9398733
     0      3200000            0     301517.9          9516230
     0      4800000            0     300357.6          9676651
     0      6400000            0     282167.8         11513255

Looks OK.

Tested-by: Dave Chinner <david@fromorbit.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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]


#1219880

FromDavidlohr Bueso <dave@stgolabs.net>
Date2015-09-07 02:10 +0200
Message-ID<q5Sat-5Rx-7@gated-at.bofh.it>
In reply to#1219116
On Fri, 04 Sep 2015, Peter Zijlstra wrote:

>-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
>+static inline bool virt_spin_lock(struct qspinlock *lock)

Given that we fall back to the cmpxchg loop even when PARAVIRT is not in the
picture, I believe this function is horribly misnamed.

> {
> 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> 		return false;
>
>-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
>-		cpu_relax();
>+	/*
>+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>+	 * back to a Test-and-Set spinlock, because fair locks have
>+	 * horrible lock 'holder' preemption issues.
>+	 */
>+

This comment is also misleading... but if you tuck the whole function
under some PARAVIRT option, it obviously makes sense to just leave as is.
And let native actually _use_ qspinlocks.

>+	do {
>+		while (atomic_read(&lock->val) != 0)
>+			cpu_relax();
>+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);

CCAS to the rescue again.

Thanks,
Davidlohr
--
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]


#1219954

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-07 09:00 +0200
Message-ID<q5Yzg-6qO-1@gated-at.bofh.it>
In reply to#1219880
On Sun, Sep 06, 2015 at 05:05:46PM -0700, Davidlohr Bueso wrote:
> On Fri, 04 Sep 2015, Peter Zijlstra wrote:
> 
> >-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
> >+static inline bool virt_spin_lock(struct qspinlock *lock)
> 
> Given that we fall back to the cmpxchg loop even when PARAVIRT is not in the
> picture, I believe this function is horribly misnamed.

Just to continue the argument for arguments sake, the function is named
'virt' (not paravirt) and tests the HYPERVISOR CPUID bit. How is that
not appropriately named?
--
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]


#1220385 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-09-07 22:50 +0200
SubjectRe: [4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q6bwt-863-15@gated-at.bofh.it>
In reply to#1219954
On Sun, Sep 6, 2015 at 11:57 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Just to continue the argument for arguments sake, the function is named
> 'virt' (not paravirt) and tests the HYPERVISOR CPUID bit. How is that
> not appropriately named?

Well, I think right now one issue is that you can't avoid it, even
when you want pure "raw hardware" spinlocks.

I really think it should at the very least be inside CONFIG_PARAVIRT.
Because it *is* about helping the hypervisor, so really is about
paravirtualization.

So naming is misleading, I think, and the config option situation is
not great. If you act differently under virtualization than you do on
raw hardware, what would you call that? I'd call it "paravirt".

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


#1220516

FromDavidlohr Bueso <dave@stgolabs.net>
Date2015-09-08 08:40 +0200
Message-ID<q6kJs-4IZ-15@gated-at.bofh.it>
In reply to#1220385
On Mon, 07 Sep 2015, Linus Torvalds wrote:

>On Sun, Sep 6, 2015 at 11:57 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> Just to continue the argument for arguments sake, the function is named
>> 'virt' (not paravirt) and tests the HYPERVISOR CPUID bit. How is that
>> not appropriately named?
>
>Well, I think right now one issue is that you can't avoid it, even
>when you want pure "raw hardware" spinlocks.
>
>I really think it should at the very least be inside CONFIG_PARAVIRT.

Yeah, I think we all agree here.

>Because it *is* about helping the hypervisor, so really is about
>paravirtualization.

Yes, this is how I interpret it as well.

CONFIG_PARAVIRT seems like a suitable place as while it is known to induce
in overhead for baremetal and distros tend to enable it by default - mainly
for mem pvops (ie: in page fault paths), and having this function doesn't
add any complexity or add make things much different than they already are.

While it is true that CONFIG_HYPERVISOR_GUEST is a pre-req for anything pv,
technically I think it's not as good a fit as CONFIG_PARAVIRT because the
former is really about describing the hypervisor, and that's not what we're
doing here.


Thanks,
Davidlohr
--
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]


#1220678

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-08 12:10 +0200
Message-ID<q6o0H-15W-71@gated-at.bofh.it>
In reply to#1220385
On Mon, Sep 07, 2015 at 01:45:18PM -0700, Linus Torvalds wrote:
> On Sun, Sep 6, 2015 at 11:57 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Just to continue the argument for arguments sake, the function is named
> > 'virt' (not paravirt) and tests the HYPERVISOR CPUID bit. How is that
> > not appropriately named?
> 
> Well, I think right now one issue is that you can't avoid it, even
> when you want pure "raw hardware" spinlocks.

The host could choose not to set the HYPERVISOR CPUID bit, but yes point
taken, and I've got a patch to make it conditional on a CONFIG thingy.

> I really think it should at the very least be inside CONFIG_PARAVIRT.
> Because it *is* about helping the hypervisor, so really is about
> paravirtualization.

Ah, so I think the confusion is in what we consider paravirt to mean. My
definition of paravirt is that its a form of virtualization where the
guest and host communicate over a software channel.

In this case; the guest does not talk to the host, there is no
hypercall, no host support required. Therefore this is not paravirt.

Wikipedia: https://en.wikipedia.org/wiki/Paravirtualization

Seems to agree with such a definition.

In any case, no strong feelings either way; the current patch that I've
got uses CONFIG_HYPERVISOR_GUEST, but I'm happy to change that to
CONFIG_PARAVIRT.


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


#1221023 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-09-08 19:50 +0200
SubjectRe: [4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q6vbQ-2PL-25@gated-at.bofh.it>
In reply to#1220678
On Tue, Sep 8, 2015 at 3:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Ah, so I think the confusion is in what we consider paravirt to mean. My
> definition of paravirt is that its a form of virtualization where the
> guest and host communicate over a software channel.

Like an architected bit in cpuid that the host sets if it wants the
guest to change its behavior?

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


#1223574 — [tip:locking/core] locking/qspinlock/x86: Fix performance regression under unaccelerated VMs

Fromtip-bot for Peter Zijlstra <tipbot@zytor.com>
Date2015-09-13 13:00 +0200
Subject[tip:locking/core] locking/qspinlock/x86: Fix performance regression under unaccelerated VMs
Message-ID<q8daO-5pP-9@gated-at.bofh.it>
In reply to#1219116
Commit-ID:  43b3f02899f74ae9914a39547cc5492156f0027a
Gitweb:     http://git.kernel.org/tip/43b3f02899f74ae9914a39547cc5492156f0027a
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 4 Sep 2015 17:25:23 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 11 Sep 2015 07:49:42 +0200

locking/qspinlock/x86: Fix performance regression under unaccelerated VMs

Dave ran into horrible performance on a VM without PARAVIRT_SPINLOCKS
set and Linus noted that the test-and-set implementation was retarded.

One should spin on the variable with a load, not a RMW.

While there, remove 'queued' from the name, as the lock isn't queued
at all, but a simple test-and-set.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Dave Chinner <david@fromorbit.com>
Tested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Waiman Long <Waiman.Long@hp.com>
Cc: stable@vger.kernel.org # v4.2+
Link: http://lkml.kernel.org/r/20150904152523.GR18673@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/qspinlock.h | 16 ++++++++++++----
 include/asm-generic/qspinlock.h  |  4 ++--
 kernel/locking/qspinlock.c       |  2 +-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 9d51fae..8dde3bd 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -39,15 +39,23 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 #endif
 
-#define virt_queued_spin_lock virt_queued_spin_lock
+#define virt_spin_lock virt_spin_lock
 
-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
+static inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
 		return false;
 
-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
-		cpu_relax();
+	/*
+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
+	 * back to a Test-and-Set spinlock, because fair locks have
+	 * horrible lock 'holder' preemption issues.
+	 */
+
+	do {
+		while (atomic_read(&lock->val) != 0)
+			cpu_relax();
+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
 
 	return true;
 }
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index 83bfb87..e2aadbc 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -111,8 +111,8 @@ static inline void queued_spin_unlock_wait(struct qspinlock *lock)
 		cpu_relax();
 }
 
-#ifndef virt_queued_spin_lock
-static __always_inline bool virt_queued_spin_lock(struct qspinlock *lock)
+#ifndef virt_spin_lock
+static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	return false;
 }
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 337c881..87e9ce6a 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -289,7 +289,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 	if (pv_enabled())
 		goto queue;
 
-	if (virt_queued_spin_lock(lock))
+	if (virt_spin_lock(lock))
 		return;
 
 	/*
--
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]


#1219120 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-09-04 17:30 +0200
SubjectRe: [4.2, Regression] Queued spinlocks cause major XFS performance regression
Message-ID<q516c-61Z-45@gated-at.bofh.it>
In reply to#1219109
On Fri, Sep 4, 2015 at 8:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> The reason we chose to revert to a test-and-set is because regular fair
> locks, like the ticket and the queue thing, have horrible behaviour
> under vcpu preemption.

Right. However, with our old ticket locks, that's what we got when you
didn't ask for paravirt support. No?

So this seems to be a misfeature - you made the hypervisor "support"
unconditional.  Even a kernel compiled for raw hardware now does that
"let's act differently under a hypervisor", which I think is quite
debatable to begin with, but when that "act differently" is then
complete garbage, it's a disaster.

And even ignoring the "implementation was crap" issue, some people may
well want their kernels to be "bare hardware" kernels even under a
hypervisor. It may be a slim hypervisor that gives you all the cpus,
or it may just be a system that is just sufficiently overprovisioned,
so you don't get vcpu preemption in practice.

But it would be interesting to hear if just fixing the busy-looping to
not pound the lock with a constant stream of cmpxchg's is already
sufficient to fix the big picture problem.

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


#1219122

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-04 17:40 +0200
Message-ID<q51fP-6d7-9@gated-at.bofh.it>
In reply to#1219120
On Fri, Sep 04, 2015 at 08:21:28AM -0700, Linus Torvalds wrote:
> On Fri, Sep 4, 2015 at 8:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > The reason we chose to revert to a test-and-set is because regular fair
> > locks, like the ticket and the queue thing, have horrible behaviour
> > under vcpu preemption.
> 
> Right. However, with our old ticket locks, that's what we got when you
> didn't ask for paravirt support. No?

Indeed.

> And even ignoring the "implementation was crap" issue, some people may
> well want their kernels to be "bare hardware" kernels even under a
> hypervisor. It may be a slim hypervisor that gives you all the cpus,
> or it may just be a system that is just sufficiently overprovisioned,
> so you don't get vcpu preemption in practice.

Fair enough; I had not considered the slim hypervisor case.

Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
hypervisor that does not support paravirt patching (HyperV, VMware,
etc..) revert to the test-and-set?

> But it would be interesting to hear if just fixing the busy-looping to
> not pound the lock with a constant stream of cmpxchg's is already
> sufficient to fix the big picture problem.

Dave replaced the cpu_relax() with a __delay(1) to match what
spinlock-debug does and that fixed things for him.

Of course, it would be good if he can try the proposed patch too.
--
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]


#1219134

FromPeter Zijlstra <peterz@infradead.org>
Date2015-09-04 18:00 +0200
Message-ID<q51zc-6zE-11@gated-at.bofh.it>
In reply to#1219122
On Fri, Sep 04, 2015 at 05:30:35PM +0200, Peter Zijlstra wrote:
> Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
> even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
> hypervisor that does not support paravirt patching (HyperV, VMware,
> etc..) revert to the test-and-set?

Ah, CONFIG_HYPERVISOR_GUEST seems fitting, that's a prerequisite for all
the PARAVIRT options too.

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


Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel


csiph-web