Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1218680 > unrolled thread
| Started by | Dave Chinner <david@fromorbit.com> |
|---|---|
| First post | 2015-09-04 07:50 +0200 |
| Last post | 2015-09-10 04:20 +0200 |
| Articles | 20 on this page of 29 — 7 participants |
Back to article view | Back to linux.kernel
[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 →
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-04 08:40 +0200 |
| Subject | Re: [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]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-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]
| From | Juergen Gross <jgross@suse.com> |
|---|---|
| Date | 2015-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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-04 17:10 +0200 |
| Subject | Re: [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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-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]
| From | Davidlohr Bueso <dave@stgolabs.net> |
|---|---|
| Date | 2015-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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-07 22:50 +0200 |
| Subject | Re: [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]
| From | Davidlohr Bueso <dave@stgolabs.net> |
|---|---|
| Date | 2015-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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-08 19:50 +0200 |
| Subject | Re: [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]
| From | tip-bot for Peter Zijlstra <tipbot@zytor.com> |
|---|---|
| Date | 2015-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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-04 17:30 +0200 |
| Subject | Re: [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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-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