* Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM
@ 2015-06-25 12:00 Pontus Fuchs
2015-06-25 12:09 ` Peter Zijlstra
0 siblings, 1 reply; 19+ messages in thread
From: Pontus Fuchs @ 2015-06-25 12:00 UTC (permalink / raw)
To: peterz; +Cc: mingo, linux-kernel
Hi,
On 4.1+ kernels I can no longer start my KVM guest. Upon trying to start
it I can see the following log message:
[ 25.821060] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:97
[ 25.821063] in_atomic(): 1, irqs_disabled(): 0, pid: 2113, name:
qemu-system-x86
[ 25.821066] CPU: 0 PID: 2113 Comm: qemu-system-x86 Not tainted 4.1.0+ #88
[ 25.821067] Hardware name: Dell Inc. Dell System XPS
15Z/00WW5M, BIOS A12 09/07/2012
[ 25.821068] 0000000000000061 ffff88021339bcd8 ffffffff816b8c81
0000000000000007
[ 25.821070] ffff880231159d40 ffff88021339bcf8 ffffffff8107d163
ffff88021339bd18
[ 25.821072] ffffffff81a451bc ffff88021339bd28 ffffffff8107d1ed
ffff8802133a0000
[ 25.821073] Call Trace:
[ 25.821078] [<ffffffff816b8c81>] dump_stack+0x4c/0x65
[ 25.821081] [<ffffffff8107d163>] ___might_sleep+0xd3/0x110
[ 25.821083] [<ffffffff8107d1ed>] __might_sleep+0x4d/0x90
[ 25.821085] [<ffffffff816bde74>] mutex_lock+0x24/0x50
[ 25.821087] [<ffffffff81141ef7>] static_key_slow_inc+0x57/0xc0
[ 25.821089] [<ffffffff8107cafd>] preempt_notifier_register+0x1d/0x60
[ 25.821099] [<ffffffffa04f11fd>] vcpu_load+0x3d/0x70 [kvm]
[ 25.821108] [<ffffffffa050699e>] kvm_arch_vcpu_setup+0x1e/0x50 [kvm]
[ 25.821115] [<ffffffffa05066e1>] ? kvm_arch_vcpu_create+0x51/0x70 [kvm]
[ 25.821120] [<ffffffffa04f29b2>] kvm_vm_ioctl+0x1d2/0x7a0 [kvm]
[ 25.821123] [<ffffffff811b7881>] do_vfs_ioctl+0x301/0x550
[ 25.821124] [<ffffffff811b7b49>] SyS_ioctl+0x79/0x90
[ 25.821127] [<ffffffff816c0257>] entry_SYSCALL_64_fastpath+0x12/0x6a
The offending commit is
commit 1cde2930e15473cb4dd7e5a07d83e605a969bd6e
Author: Peter Zijlstra <peterz@infradead.org>
Date: Mon Jun 8 16:00:30 2015 +0200
sched/preempt: Add static_key() to preempt_notifiers
BR,
Pontus Fuchs
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM 2015-06-25 12:00 Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Pontus Fuchs @ 2015-06-25 12:09 ` Peter Zijlstra 2015-06-25 12:15 ` Pontus Fuchs 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2015-06-25 12:09 UTC (permalink / raw) To: Pontus Fuchs; +Cc: mingo, linux-kernel, gleb On Thu, Jun 25, 2015 at 02:00:02PM +0200, Pontus Fuchs wrote: > Hi, > > On 4.1+ kernels I can no longer start my KVM guest. Upon trying to start it > I can see the following log message: > > [ 25.821060] BUG: sleeping function called from invalid context at > kernel/locking/mutex.c:97 > [ 25.821063] in_atomic(): 1, irqs_disabled(): 0, pid: 2113, name: > qemu-system-x86 > [ 25.821066] CPU: 0 PID: 2113 Comm: qemu-system-x86 Not tainted 4.1.0+ #88 > [ 25.821067] Hardware name: Dell Inc. Dell System XPS 15Z/00WW5M, > BIOS A12 09/07/2012 > [ 25.821068] 0000000000000061 ffff88021339bcd8 ffffffff816b8c81 > 0000000000000007 > [ 25.821070] ffff880231159d40 ffff88021339bcf8 ffffffff8107d163 > ffff88021339bd18 > [ 25.821072] ffffffff81a451bc ffff88021339bd28 ffffffff8107d1ed > ffff8802133a0000 > [ 25.821073] Call Trace: > [ 25.821078] [<ffffffff816b8c81>] dump_stack+0x4c/0x65 > [ 25.821081] [<ffffffff8107d163>] ___might_sleep+0xd3/0x110 > [ 25.821083] [<ffffffff8107d1ed>] __might_sleep+0x4d/0x90 > [ 25.821085] [<ffffffff816bde74>] mutex_lock+0x24/0x50 > [ 25.821087] [<ffffffff81141ef7>] static_key_slow_inc+0x57/0xc0 > [ 25.821089] [<ffffffff8107cafd>] preempt_notifier_register+0x1d/0x60 > [ 25.821099] [<ffffffffa04f11fd>] vcpu_load+0x3d/0x70 [kvm] > [ 25.821108] [<ffffffffa050699e>] kvm_arch_vcpu_setup+0x1e/0x50 [kvm] > [ 25.821115] [<ffffffffa05066e1>] ? kvm_arch_vcpu_create+0x51/0x70 [kvm] > [ 25.821120] [<ffffffffa04f29b2>] kvm_vm_ioctl+0x1d2/0x7a0 [kvm] > [ 25.821123] [<ffffffff811b7881>] do_vfs_ioctl+0x301/0x550 > [ 25.821124] [<ffffffff811b7b49>] SyS_ioctl+0x79/0x90 > [ 25.821127] [<ffffffff816c0257>] entry_SYSCALL_64_fastpath+0x12/0x6a > That seems pointless.. --- virt/kvm/kvm_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 90977418aeb6..d7aafa0458a0 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) if (mutex_lock_killable(&vcpu->mutex)) return -EINTR; - cpu = get_cpu(); preempt_notifier_register(&vcpu->preempt_notifier); + + cpu = get_cpu(); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); return 0; @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) { preempt_disable(); kvm_arch_vcpu_put(vcpu); - preempt_notifier_unregister(&vcpu->preempt_notifier); preempt_enable(); + preempt_notifier_unregister(&vcpu->preempt_notifier); mutex_unlock(&vcpu->mutex); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM 2015-06-25 12:09 ` Peter Zijlstra @ 2015-06-25 12:15 ` Pontus Fuchs 2015-06-25 12:55 ` [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra 2015-06-30 13:47 ` Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Josh Boyer 0 siblings, 2 replies; 19+ messages in thread From: Pontus Fuchs @ 2015-06-25 12:15 UTC (permalink / raw) To: Peter Zijlstra; +Cc: mingo, linux-kernel, gleb On 2015-06-25 14:09, Peter Zijlstra wrote: > On Thu, Jun 25, 2015 at 02:00:02PM +0200, Pontus Fuchs wrote: >> Hi, >> > > That seems pointless.. > > --- > virt/kvm/kvm_main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 90977418aeb6..d7aafa0458a0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > + > + cpu = get_cpu(); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > + preempt_notifier_unregister(&vcpu->preempt_notifier); > mutex_unlock(&vcpu->mutex); > } Tested ok. Thanks. BR, Pontus ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-06-25 12:15 ` Pontus Fuchs @ 2015-06-25 12:55 ` Peter Zijlstra 2015-06-30 11:10 ` [tip:sched/urgent] sched/preempt, kvm: " tip-bot for Peter Zijlstra 2015-07-03 11:12 ` [PATCH] sched,kvm: " Paolo Bonzini 2015-06-30 13:47 ` Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Josh Boyer 1 sibling, 2 replies; 19+ messages in thread From: Peter Zijlstra @ 2015-06-25 12:55 UTC (permalink / raw) To: Pontus Fuchs; +Cc: mingo, linux-kernel, gleb Subject: sched,kvm: Fix KVM preempt_notifier usage The preempt-notifier API needs to sleep with the addition of the static_key, we do however need to hold off preemption while modifying the preempt notifier list, otherwise a preemption could observe an inconsistent list state. There is no reason to have preemption disabled in the caller, registering a preempt notifier is a purely task local affair. Therefore move the preempt_disable into the functions and change the callers to a preemptible context. Cc: Gleb Natapov <gleb@kernel.org> Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Reported-and-Tested-by: Pontus Fuchs <pontus.fuchs@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/sched/core.c | 11 +++++++++++ virt/kvm/kvm_main.c | 5 +++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index b803e1b8ab0c..6169c167ac98 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2327,7 +2327,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; void preempt_notifier_register(struct preempt_notifier *notifier) { static_key_slow_inc(&preempt_notifier_key); + /* + * Avoid preemption while changing the preempt notifier list. + */ + preempt_disable(); hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); + preempt_enable(); } EXPORT_SYMBOL_GPL(preempt_notifier_register); @@ -2339,7 +2344,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); */ void preempt_notifier_unregister(struct preempt_notifier *notifier) { + /* + * Avoid preemption while changing the preempt notifier list. + */ + preempt_disable(); hlist_del(¬ifier->link); + preempt_enable(); + static_key_slow_dec(&preempt_notifier_key); } EXPORT_SYMBOL_GPL(preempt_notifier_unregister); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 90977418aeb6..d7aafa0458a0 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) if (mutex_lock_killable(&vcpu->mutex)) return -EINTR; - cpu = get_cpu(); preempt_notifier_register(&vcpu->preempt_notifier); + + cpu = get_cpu(); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); return 0; @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) { preempt_disable(); kvm_arch_vcpu_put(vcpu); - preempt_notifier_unregister(&vcpu->preempt_notifier); preempt_enable(); + preempt_notifier_unregister(&vcpu->preempt_notifier); mutex_unlock(&vcpu->mutex); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [tip:sched/urgent] sched/preempt, kvm: Fix KVM preempt_notifier usage 2015-06-25 12:55 ` [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra @ 2015-06-30 11:10 ` tip-bot for Peter Zijlstra 2015-07-03 11:23 ` Paolo Bonzini 2015-07-03 11:12 ` [PATCH] sched,kvm: " Paolo Bonzini 1 sibling, 1 reply; 19+ messages in thread From: tip-bot for Peter Zijlstra @ 2015-06-30 11:10 UTC (permalink / raw) To: linux-tip-commits Cc: peterz, tglx, linux-kernel, bp, gleb, akpm, torvalds, hpa, pontus.fuchs, mingo Commit-ID: 6efde1d3716b7d055d3310bccba2df244cf430d7 Gitweb: http://git.kernel.org/tip/6efde1d3716b7d055d3310bccba2df244cf430d7 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Thu, 25 Jun 2015 14:55:14 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Tue, 30 Jun 2015 07:58:53 +0200 sched/preempt, kvm: Fix KVM preempt_notifier usage The preempt-notifier API needs to sleep with the addition of the static_key, we do however need to hold off preemption while modifying the preempt notifier list, otherwise a preemption could observe an inconsistent list state. There is no reason to have preemption disabled in the caller, registering a preempt notifier is a purely task local affair. Therefore move the preempt_disable into the functions and change the callers to a preemptible context. Reported-and-Tested-by: Pontus Fuchs <pontus.fuchs@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Borislav Petkov <bp@alien8.de> Cc: Gleb Natapov <gleb@kernel.org> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Pontus Fuchs <pontus.fuchs@gmail.com> Cc: Thomas Gleixner <tglx@linutronix.de> Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Link: http://lkml.kernel.org/r/20150625125514.GA3644@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/sched/core.c | 11 +++++++++++ virt/kvm/kvm_main.c | 5 +++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index c86935a..5bcf926 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2138,7 +2138,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; void preempt_notifier_register(struct preempt_notifier *notifier) { static_key_slow_inc(&preempt_notifier_key); + /* + * Avoid preemption while changing the preempt notifier list. + */ + preempt_disable(); hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); + preempt_enable(); } EXPORT_SYMBOL_GPL(preempt_notifier_register); @@ -2150,7 +2155,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); */ void preempt_notifier_unregister(struct preempt_notifier *notifier) { + /* + * Avoid preemption while changing the preempt notifier list. + */ + preempt_disable(); hlist_del(¬ifier->link); + preempt_enable(); + static_key_slow_dec(&preempt_notifier_key); } EXPORT_SYMBOL_GPL(preempt_notifier_unregister); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 9097741..d7aafa0 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) if (mutex_lock_killable(&vcpu->mutex)) return -EINTR; - cpu = get_cpu(); preempt_notifier_register(&vcpu->preempt_notifier); + + cpu = get_cpu(); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); return 0; @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) { preempt_disable(); kvm_arch_vcpu_put(vcpu); - preempt_notifier_unregister(&vcpu->preempt_notifier); preempt_enable(); + preempt_notifier_unregister(&vcpu->preempt_notifier); mutex_unlock(&vcpu->mutex); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [tip:sched/urgent] sched/preempt, kvm: Fix KVM preempt_notifier usage 2015-06-30 11:10 ` [tip:sched/urgent] sched/preempt, kvm: " tip-bot for Peter Zijlstra @ 2015-07-03 11:23 ` Paolo Bonzini 0 siblings, 0 replies; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 11:23 UTC (permalink / raw) To: mingo, hpa, pontus.fuchs, peterz, gleb, bp, linux-kernel, tglx, torvalds, akpm, linux-tip-commits On 30/06/2015 13:10, tip-bot for Peter Zijlstra wrote: > Commit-ID: 6efde1d3716b7d055d3310bccba2df244cf430d7 > Gitweb: http://git.kernel.org/tip/6efde1d3716b7d055d3310bccba2df244cf430d7 > Author: Peter Zijlstra <peterz@infradead.org> > AuthorDate: Thu, 25 Jun 2015 14:55:14 +0200 > Committer: Ingo Molnar <mingo@kernel.org> > CommitDate: Tue, 30 Jun 2015 07:58:53 +0200 > > sched/preempt, kvm: Fix KVM preempt_notifier usage > > The preempt-notifier API needs to sleep with the addition of the > static_key, we do however need to hold off preemption while > modifying the preempt notifier list, otherwise a preemption > could observe an inconsistent list state. > > There is no reason to have preemption disabled in the caller, > registering a preempt notifier is a purely task local affair. > > Therefore move the preempt_disable into the functions and change > the callers to a preemptible context. > > Reported-and-Tested-by: Pontus Fuchs <pontus.fuchs@gmail.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Gleb Natapov <gleb@kernel.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Pontus Fuchs <pontus.fuchs@gmail.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") > Link: http://lkml.kernel.org/r/20150625125514.GA3644@twins.programming.kicks-ass.net > Signed-off-by: Ingo Molnar <mingo@kernel.org> > --- > kernel/sched/core.c | 11 +++++++++++ > virt/kvm/kvm_main.c | 5 +++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index c86935a..5bcf926 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2138,7 +2138,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; > void preempt_notifier_register(struct preempt_notifier *notifier) > { > static_key_slow_inc(&preempt_notifier_key); > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); > + preempt_enable(); > } > EXPORT_SYMBOL_GPL(preempt_notifier_register); > > @@ -2150,7 +2155,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); > */ > void preempt_notifier_unregister(struct preempt_notifier *notifier) > { > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_del(¬ifier->link); > + preempt_enable(); > + > static_key_slow_dec(&preempt_notifier_key); > } > EXPORT_SYMBOL_GPL(preempt_notifier_unregister); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 9097741..d7aafa0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > + > + cpu = get_cpu(); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > + preempt_notifier_unregister(&vcpu->preempt_notifier); > mutex_unlock(&vcpu->mutex); > } > > NACK. Paolo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-06-25 12:55 ` [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra 2015-06-30 11:10 ` [tip:sched/urgent] sched/preempt, kvm: " tip-bot for Peter Zijlstra @ 2015-07-03 11:12 ` Paolo Bonzini 2015-07-03 12:19 ` Peter Zijlstra 1 sibling, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 11:12 UTC (permalink / raw) To: Peter Zijlstra, Pontus Fuchs, Linus Torvalds; +Cc: mingo, linux-kernel, gleb On 25/06/2015 14:55, Peter Zijlstra wrote: > Subject: sched,kvm: Fix KVM preempt_notifier usage > > The preempt-notifier API needs to sleep with the addition of the > static_key, we do however need to hold off preemption while modifying > the preempt notifier list, otherwise a preemption could observe an > inconsistent list state. > > There is no reason to have preemption disabled in the caller, > registering a preempt notifier is a purely task local affair. > > Therefore move the preempt_disable into the functions and change the > callers to a preemptible context. > > Cc: Gleb Natapov <gleb@kernel.org> Uhm, you forgot at least me and the KVM mailing list. And you didn't even Cc Gleb on the original patch. So nice of you, and it makes me wonder if you even grepped for preempt_notifier_register when you wrote the original patch. Probably not, since you couldn't even be bothered to test the *only* user of preempt notifiers. In fact you shouldn't have just tested the patch on a case _without_ preemption notifiers, you should have also benchmarked the impact that static keys have _with_ preemption notifiers. In a not-really-artificial case (one single-processor guest running on the host), the static key patch adds a static_key_slow_inc on a relatively hot path for KVM, which is not acceptable. So, NACK. > Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Linus, can you please revert the above patch instead? Paolo > Reported-and-Tested-by: Pontus Fuchs <pontus.fuchs@gmail.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/sched/core.c | 11 +++++++++++ > virt/kvm/kvm_main.c | 5 +++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index b803e1b8ab0c..6169c167ac98 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2327,7 +2327,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; > void preempt_notifier_register(struct preempt_notifier *notifier) > { > static_key_slow_inc(&preempt_notifier_key); > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); > + preempt_enable(); > } > EXPORT_SYMBOL_GPL(preempt_notifier_register); > > @@ -2339,7 +2344,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); > */ > void preempt_notifier_unregister(struct preempt_notifier *notifier) > { > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_del(¬ifier->link); > + preempt_enable(); > + > static_key_slow_dec(&preempt_notifier_key); > } > EXPORT_SYMBOL_GPL(preempt_notifier_unregister); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 90977418aeb6..d7aafa0458a0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > + > + cpu = get_cpu(); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > + preempt_notifier_unregister(&vcpu->preempt_notifier); > mutex_unlock(&vcpu->mutex); > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 11:12 ` [PATCH] sched,kvm: " Paolo Bonzini @ 2015-07-03 12:19 ` Peter Zijlstra 2015-07-03 12:31 ` Paolo Bonzini 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2015-07-03 12:19 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On Fri, Jul 03, 2015 at 01:12:11PM +0200, Paolo Bonzini wrote: > In fact you shouldn't have just tested the patch on a case _without_ > preemption notifiers, you should have also benchmarked the impact that > static keys have _with_ preemption notifiers. In a > not-really-artificial case (one single-processor guest running on the > host), the static key patch adds a static_key_slow_inc on a relatively > hot path for KVM, which is not acceptable. Spawning the first vcpu is a hot path? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 12:19 ` Peter Zijlstra @ 2015-07-03 12:31 ` Paolo Bonzini 2015-07-03 13:17 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 12:31 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On 03/07/2015 14:19, Peter Zijlstra wrote: > On Fri, Jul 03, 2015 at 01:12:11PM +0200, Paolo Bonzini wrote: >> In fact you shouldn't have just tested the patch on a case _without_ >> preemption notifiers, you should have also benchmarked the impact that >> static keys have _with_ preemption notifiers. In a >> not-really-artificial case (one single-processor guest running on the >> host), the static key patch adds a static_key_slow_inc on a relatively >> hot path for KVM, which is not acceptable. > > Spawning the first vcpu is a hot path? This is not *spawning* the first VCPU. Basically any critical section for vcpu->mutex includes a preempt_notifier_register/unregister pair: /* * Switches to specified vcpu, until a matching vcpu_put() */ int vcpu_load(struct kvm_vcpu *vcpu) { int cpu; if (mutex_lock_killable(&vcpu->mutex)) return -EINTR; cpu = get_cpu(); preempt_notifier_register(&vcpu->preempt_notifier); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); return 0; } void vcpu_put(struct kvm_vcpu *vcpu) { preempt_disable(); kvm_arch_vcpu_put(vcpu); preempt_notifier_unregister(&vcpu->preempt_notifier); preempt_enable(); mutex_unlock(&vcpu->mutex); } So basically you're adding at least one static_key_slow_inc/dec pair to every userspace exit. Paolo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 12:31 ` Paolo Bonzini @ 2015-07-03 13:17 ` Peter Zijlstra 2015-07-03 15:16 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2015-07-03 13:17 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On Fri, Jul 03, 2015 at 02:31:25PM +0200, Paolo Bonzini wrote: > On 03/07/2015 14:19, Peter Zijlstra wrote: > > On Fri, Jul 03, 2015 at 01:12:11PM +0200, Paolo Bonzini wrote: > >> In fact you shouldn't have just tested the patch on a case _without_ > >> preemption notifiers, you should have also benchmarked the impact that > >> static keys have _with_ preemption notifiers. In a > >> not-really-artificial case (one single-processor guest running on the > >> host), the static key patch adds a static_key_slow_inc on a relatively > >> hot path for KVM, which is not acceptable. > > > > Spawning the first vcpu is a hot path? > > This is not *spawning* the first VCPU. Basically any critical section > for vcpu->mutex includes a preempt_notifier_register/unregister pair: > > /* > * Switches to specified vcpu, until a matching vcpu_put() > */ > int vcpu_load(struct kvm_vcpu *vcpu) > { > int cpu; > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > } > > void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > mutex_unlock(&vcpu->mutex); > } > > So basically you're adding at least one static_key_slow_inc/dec pair to > every userspace exit. Ugh, ok that is not what I was expecting to happen. I'll ask Ingo to queue a revert until we can fix this better. I thought these were vcpu create/destroy functions. That said, the slow_inc/dec are really only slow on the 0<->!0 transitions. But, could we rework the code so that you register the preempt notifier when creating the vcpu thread and leave it installed forevermore? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 13:17 ` Peter Zijlstra @ 2015-07-03 15:16 ` Peter Zijlstra 2015-07-03 15:26 ` Paolo Bonzini 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2015-07-03 15:16 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On Fri, Jul 03, 2015 at 03:17:13PM +0200, Peter Zijlstra wrote: > But, could we rework the code so that you register the preempt notifier > when creating the vcpu thread and leave it installed forevermore? OK, it looks like there is no fixed relation between a thread and a vcpu :/ Would something like the below (on top of all the others) work for you? --- include/linux/preempt.h | 2 ++ kernel/sched/core.c | 18 +++++++++++++++--- virt/kvm/kvm_main.c | 7 +++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/include/linux/preempt.h b/include/linux/preempt.h index 0f1534acaf60..84991f185173 100644 --- a/include/linux/preempt.h +++ b/include/linux/preempt.h @@ -293,6 +293,8 @@ struct preempt_notifier { struct preempt_ops *ops; }; +void preempt_notifier_inc(void); +void preempt_notifier_dec(void); void preempt_notifier_register(struct preempt_notifier *notifier); void preempt_notifier_unregister(struct preempt_notifier *notifier); diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 6169c167ac98..5ddbcb720fc6 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2320,13 +2320,27 @@ void wake_up_new_task(struct task_struct *p) static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; +void preempt_notifier_inc(void) +{ + static_key_slow_inc(&preempt_notifier_key); +} +EXPORT_SYMBOL_GPL(preempt_notifier_inc); + +void preempt_notifier_dec(void) +{ + static_key_slow_dec(&preempt_notifier_key); +} +EXPORT_SYMBOL_GPL(preempt_notifier_dec); + /** * preempt_notifier_register - tell me when current is being preempted & rescheduled * @notifier: notifier struct to register */ void preempt_notifier_register(struct preempt_notifier *notifier) { - static_key_slow_inc(&preempt_notifier_key); + if (!static_key_false(&preempt_notifier_key)) + WARN(1, "registering preempt_notifier while notifiers disabled\n"); + /* * Avoid preemption while changing the preempt notifier list. */ @@ -2350,8 +2364,6 @@ void preempt_notifier_unregister(struct preempt_notifier *notifier) preempt_disable(); hlist_del(¬ifier->link); preempt_enable(); - - static_key_slow_dec(&preempt_notifier_key); } EXPORT_SYMBOL_GPL(preempt_notifier_unregister); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index d7aafa0458a0..8136be28d76c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -128,9 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) if (mutex_lock_killable(&vcpu->mutex)) return -EINTR; - preempt_notifier_register(&vcpu->preempt_notifier); cpu = get_cpu(); + preempt_notifier_register(&vcpu->preempt_notifier); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); return 0; @@ -140,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) { preempt_disable(); kvm_arch_vcpu_put(vcpu); - preempt_enable(); preempt_notifier_unregister(&vcpu->preempt_notifier); + preempt_enable(); mutex_unlock(&vcpu->mutex); } @@ -513,6 +513,8 @@ static struct kvm *kvm_create_vm(unsigned long type) list_add(&kvm->vm_list, &vm_list); spin_unlock(&kvm_lock); + preempt_notifier_inc(); + return kvm; out_err: @@ -612,6 +614,7 @@ static void kvm_destroy_vm(struct kvm *kvm) cleanup_srcu_struct(&kvm->irq_srcu); cleanup_srcu_struct(&kvm->srcu); kvm_arch_free_vm(kvm); + preempt_notifier_dec(); hardware_disable_all(); mmdrop(mm); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 15:16 ` Peter Zijlstra @ 2015-07-03 15:26 ` Paolo Bonzini 2015-07-03 15:38 ` Paolo Bonzini 0 siblings, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 15:26 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On 03/07/2015 17:16, Peter Zijlstra wrote: > On Fri, Jul 03, 2015 at 03:17:13PM +0200, Peter Zijlstra wrote: >> But, could we rework the code so that you register the preempt notifier >> when creating the vcpu thread and leave it installed forevermore? > > OK, it looks like there is no fixed relation between a thread and a vcpu > :/ > > Would something like the below (on top of all the others) work for you? This looks fine, but one would do the same thing without the previous patch, i.e. directly on top of Linus's master---right? The patch in tip is a red herring, and the hunks below are reverting large parts of it. I'm going to send a pull request to Linus anyway, either tonight or tomorrow morning. Send it with SoB, so that it applies to Linus's master, and I can include it. This way bisection is also better preserved. Paolo > --- > include/linux/preempt.h | 2 ++ > kernel/sched/core.c | 18 +++++++++++++++--- > virt/kvm/kvm_main.c | 7 +++++-- > 3 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/linux/preempt.h b/include/linux/preempt.h > index 0f1534acaf60..84991f185173 100644 > --- a/include/linux/preempt.h > +++ b/include/linux/preempt.h > @@ -293,6 +293,8 @@ struct preempt_notifier { > struct preempt_ops *ops; > }; > > +void preempt_notifier_inc(void); > +void preempt_notifier_dec(void); > void preempt_notifier_register(struct preempt_notifier *notifier); > void preempt_notifier_unregister(struct preempt_notifier *notifier); > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 6169c167ac98..5ddbcb720fc6 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2320,13 +2320,27 @@ void wake_up_new_task(struct task_struct *p) > > static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; > > +void preempt_notifier_inc(void) > +{ > + static_key_slow_inc(&preempt_notifier_key); > +} > +EXPORT_SYMBOL_GPL(preempt_notifier_inc); > + > +void preempt_notifier_dec(void) > +{ > + static_key_slow_dec(&preempt_notifier_key); > +} > +EXPORT_SYMBOL_GPL(preempt_notifier_dec); > + > /** > * preempt_notifier_register - tell me when current is being preempted & rescheduled > * @notifier: notifier struct to register > */ > void preempt_notifier_register(struct preempt_notifier *notifier) > { > - static_key_slow_inc(&preempt_notifier_key); > + if (!static_key_false(&preempt_notifier_key)) > + WARN(1, "registering preempt_notifier while notifiers disabled\n"); > + > /* > * Avoid preemption while changing the preempt notifier list. > */ > @@ -2350,8 +2364,6 @@ void preempt_notifier_unregister(struct preempt_notifier *notifier) > preempt_disable(); > hlist_del(¬ifier->link); > preempt_enable(); > - > - static_key_slow_dec(&preempt_notifier_key); > } > EXPORT_SYMBOL_GPL(preempt_notifier_unregister); > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index d7aafa0458a0..8136be28d76c 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,9 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - preempt_notifier_register(&vcpu->preempt_notifier); > > cpu = get_cpu(); > + preempt_notifier_register(&vcpu->preempt_notifier); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -140,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_enable(); > preempt_notifier_unregister(&vcpu->preempt_notifier); > + preempt_enable(); > mutex_unlock(&vcpu->mutex); > } > > @@ -513,6 +513,8 @@ static struct kvm *kvm_create_vm(unsigned long type) > list_add(&kvm->vm_list, &vm_list); > spin_unlock(&kvm_lock); > > + preempt_notifier_inc(); > + > return kvm; > > out_err: > @@ -612,6 +614,7 @@ static void kvm_destroy_vm(struct kvm *kvm) > cleanup_srcu_struct(&kvm->irq_srcu); > cleanup_srcu_struct(&kvm->srcu); > kvm_arch_free_vm(kvm); > + preempt_notifier_dec(); > hardware_disable_all(); > mmdrop(mm); > } > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 15:26 ` Paolo Bonzini @ 2015-07-03 15:38 ` Paolo Bonzini 2015-07-03 15:42 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 15:38 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On 03/07/2015 17:26, Paolo Bonzini wrote: > > > On 03/07/2015 17:16, Peter Zijlstra wrote: >> On Fri, Jul 03, 2015 at 03:17:13PM +0200, Peter Zijlstra wrote: >>> But, could we rework the code so that you register the preempt notifier >>> when creating the vcpu thread and leave it installed forevermore? >> >> OK, it looks like there is no fixed relation between a thread and a vcpu >> :/ >> >> Would something like the below (on top of all the others) work for you? > > This looks fine, but one would do the same thing without the previous > patch, i.e. directly on top of Linus's master---right? The patch in tip > is a red herring, and the hunks below are reverting large parts of it. So basically this. Can you reply with SoB and maybe Acked-by? ------------- 8< --------------- From: Peter Zijlstra <peterz@infradead.org> Subject: [PATCH] sched, preempt_notifier: separate notifier registration from static_key inc/dec Commit 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") had two problems. First, the preempt-notifier API needs to sleep with the addition of the static_key, we do however need to hold off preemption while modifying the preempt notifier list, otherwise a preemption could observe an inconsistent list state. KVM correctly registers and unregisters preempt notifiers with preemption disabled, so the sleep caused dmesg splats. Second, KVM registers and unregisters preemption notifiers very often (in vcpu_load/vcpu_put). With a single uniprocessor guest the static key would move between 0 and 1 continuously, hitting the slow path on every userspace exit. To fix this, wrap the static_key inc/dec in a new API, and call it from KVM. Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Reported-by: Pontus Fuchs <pontus.fuchs@gmail.com> Reported-by: Takashi Iwai <tiwai@suse.de> Not-signed-off-by: Peter Zijlstra <peterz@infradead.org> Not-acked-by: Peter Zijlstra <peterz@infradead.org> Not-signed-off-by: Paolo Bonzini <pbonzini@redhat.com diff --git a/include/linux/preempt.h b/include/linux/preempt.h index 0f1534acaf60..84991f185173 100644 --- a/include/linux/preempt.h +++ b/include/linux/preempt.h @@ -293,6 +293,8 @@ struct preempt_notifier { struct preempt_ops *ops; }; +void preempt_notifier_inc(void); +void preempt_notifier_dec(void); void preempt_notifier_register(struct preempt_notifier *notifier); void preempt_notifier_unregister(struct preempt_notifier *notifier); diff --git a/kernel/sched/core.c b/kernel/sched/core.c index b803e1b8ab0c..552710ab19e0 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2320,13 +2320,27 @@ void wake_up_new_task(struct task_struct *p) static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; +void preempt_notifier_inc(void) +{ + static_key_slow_inc(&preempt_notifier_key); +} +EXPORT_SYMBOL_GPL(preempt_notifier_inc); + +void preempt_notifier_dec(void) +{ + static_key_slow_dec(&preempt_notifier_key); +} +EXPORT_SYMBOL_GPL(preempt_notifier_dec); + /** * preempt_notifier_register - tell me when current is being preempted & rescheduled * @notifier: notifier struct to register */ void preempt_notifier_register(struct preempt_notifier *notifier) { - static_key_slow_inc(&preempt_notifier_key); + if (!static_key_false(&preempt_notifier_key)) + WARN(1, "registering preempt_notifier while notifiers disabled\n"); + hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); } EXPORT_SYMBOL_GPL(preempt_notifier_register); @@ -2340,7 +2354,6 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); void preempt_notifier_unregister(struct preempt_notifier *notifier) { hlist_del(¬ifier->link); - static_key_slow_dec(&preempt_notifier_key); } EXPORT_SYMBOL_GPL(preempt_notifier_unregister); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 848af90b8091..8b8a44453670 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -553,6 +553,8 @@ static struct kvm *kvm_create_vm(unsigned long type) list_add(&kvm->vm_list, &vm_list); spin_unlock(&kvm_lock); + preempt_notifier_inc(); + return kvm; out_err: @@ -620,6 +622,7 @@ static void kvm_destroy_vm(struct kvm *kvm) cleanup_srcu_struct(&kvm->irq_srcu); cleanup_srcu_struct(&kvm->srcu); kvm_arch_free_vm(kvm); + preempt_notifier_dec(); hardware_disable_all(); mmdrop(mm); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 15:38 ` Paolo Bonzini @ 2015-07-03 15:42 ` Peter Zijlstra 2015-07-03 15:46 ` Paolo Bonzini 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2015-07-03 15:42 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On Fri, Jul 03, 2015 at 05:38:42PM +0200, Paolo Bonzini wrote: > So basically this. Can you reply with SoB and maybe Acked-by? Ah, thanks for doing that! > ------------- 8< --------------- > From: Peter Zijlstra <peterz@infradead.org> > Subject: [PATCH] sched, preempt_notifier: separate notifier registration from static_key inc/dec > > Commit 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") > had two problems. First, the preempt-notifier API needs to sleep with the > addition of the static_key, we do however need to hold off preemption > while modifying the preempt notifier list, otherwise a preemption could > observe an inconsistent list state. KVM correctly registers and > unregisters preempt notifiers with preemption disabled, so the sleep > caused dmesg splats. > > Second, KVM registers and unregisters preemption notifiers very often > (in vcpu_load/vcpu_put). With a single uniprocessor guest the static key > would move between 0 and 1 continuously, hitting the slow path on every > userspace exit. > > To fix this, wrap the static_key inc/dec in a new API, and call it from > KVM. > > Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") > Reported-by: Pontus Fuchs <pontus.fuchs@gmail.com> > Reported-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 15:42 ` Peter Zijlstra @ 2015-07-03 15:46 ` Paolo Bonzini 2015-07-03 15:57 ` Takashi Iwai 0 siblings, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2015-07-03 15:46 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb On 03/07/2015 17:42, Peter Zijlstra wrote: > On Fri, Jul 03, 2015 at 05:38:42PM +0200, Paolo Bonzini wrote: >> So basically this. Can you reply with SoB and maybe Acked-by? > > Ah, thanks for doing that! > >> ------------- 8< --------------- >> From: Peter Zijlstra <peterz@infradead.org> >> Subject: [PATCH] sched, preempt_notifier: separate notifier registration from static_key inc/dec >> >> Commit 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") >> had two problems. First, the preempt-notifier API needs to sleep with the >> addition of the static_key, we do however need to hold off preemption >> while modifying the preempt notifier list, otherwise a preemption could >> observe an inconsistent list state. KVM correctly registers and >> unregisters preempt notifiers with preemption disabled, so the sleep >> caused dmesg splats. >> >> Second, KVM registers and unregisters preemption notifiers very often >> (in vcpu_load/vcpu_put). With a single uniprocessor guest the static key >> would move between 0 and 1 continuously, hitting the slow path on every >> userspace exit. >> >> To fix this, wrap the static_key inc/dec in a new API, and call it from >> KVM. >> >> Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") >> Reported-by: Pontus Fuchs <pontus.fuchs@gmail.com> >> Reported-by: Takashi Iwai <tiwai@suse.de> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Ok, I'm crossing fingers and including this in my pull request in order to preserve bisectability. Thanks. Paolo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage 2015-07-03 15:46 ` Paolo Bonzini @ 2015-07-03 15:57 ` Takashi Iwai 0 siblings, 0 replies; 19+ messages in thread From: Takashi Iwai @ 2015-07-03 15:57 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Zijlstra, Pontus Fuchs, Linus Torvalds, mingo, linux-kernel, gleb At Fri, 3 Jul 2015 17:46:06 +0200, Paolo Bonzini wrote: > > > > On 03/07/2015 17:42, Peter Zijlstra wrote: > > On Fri, Jul 03, 2015 at 05:38:42PM +0200, Paolo Bonzini wrote: > >> So basically this. Can you reply with SoB and maybe Acked-by? > > > > Ah, thanks for doing that! > > > >> ------------- 8< --------------- > >> From: Peter Zijlstra <peterz@infradead.org> > >> Subject: [PATCH] sched, preempt_notifier: separate notifier registration from static_key inc/dec > >> > >> Commit 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") > >> had two problems. First, the preempt-notifier API needs to sleep with the > >> addition of the static_key, we do however need to hold off preemption > >> while modifying the preempt notifier list, otherwise a preemption could > >> observe an inconsistent list state. KVM correctly registers and > >> unregisters preempt notifiers with preemption disabled, so the sleep > >> caused dmesg splats. > >> > >> Second, KVM registers and unregisters preemption notifiers very often > >> (in vcpu_load/vcpu_put). With a single uniprocessor guest the static key > >> would move between 0 and 1 continuously, hitting the slow path on every > >> userspace exit. > >> > >> To fix this, wrap the static_key inc/dec in a new API, and call it from > >> KVM. > >> > >> Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") > >> Reported-by: Pontus Fuchs <pontus.fuchs@gmail.com> > >> Reported-by: Takashi Iwai <tiwai@suse.de> > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > Ok, I'm crossing fingers and including this in my pull request in order > to preserve bisectability. Thanks. I checked the patch now and confirmed that it fixes the regressions (the warnings and the sluggish mouse pointer). Feel free to my tested-by tag, if any. Tested-by: Takashi Iwai <tiwai@suse.de> Thanks! Takashi ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM 2015-06-25 12:15 ` Pontus Fuchs 2015-06-25 12:55 ` [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra @ 2015-06-30 13:47 ` Josh Boyer 2015-07-01 6:55 ` Ingo Molnar 1 sibling, 1 reply; 19+ messages in thread From: Josh Boyer @ 2015-06-30 13:47 UTC (permalink / raw) To: Pontus Fuchs; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Gleb Natapov On Thu, Jun 25, 2015 at 8:15 AM, Pontus Fuchs <pontus.fuchs@gmail.com> wrote: > On 2015-06-25 14:09, Peter Zijlstra wrote: >> >> On Thu, Jun 25, 2015 at 02:00:02PM +0200, Pontus Fuchs wrote: >>> >>> Hi, >>> >> >> That seems pointless.. >> >> --- >> virt/kvm/kvm_main.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 90977418aeb6..d7aafa0458a0 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) >> >> if (mutex_lock_killable(&vcpu->mutex)) >> return -EINTR; >> - cpu = get_cpu(); >> preempt_notifier_register(&vcpu->preempt_notifier); >> + >> + cpu = get_cpu(); >> kvm_arch_vcpu_load(vcpu, cpu); >> put_cpu(); >> return 0; >> @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) >> { >> preempt_disable(); >> kvm_arch_vcpu_put(vcpu); >> - preempt_notifier_unregister(&vcpu->preempt_notifier); >> preempt_enable(); >> + preempt_notifier_unregister(&vcpu->preempt_notifier); >> mutex_unlock(&vcpu->mutex); >> } > > > Tested ok. Thanks. We've had a report of this in Fedora now. Is the above patch queued anywhere? https://bugzilla.redhat.com/show_bug.cgi?id=1237143 josh ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM 2015-06-30 13:47 ` Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Josh Boyer @ 2015-07-01 6:55 ` Ingo Molnar 2015-07-03 13:15 ` Takashi Iwai 0 siblings, 1 reply; 19+ messages in thread From: Ingo Molnar @ 2015-07-01 6:55 UTC (permalink / raw) To: Josh Boyer Cc: Pontus Fuchs, Peter Zijlstra, Ingo Molnar, linux-kernel, Gleb Natapov * Josh Boyer <jwboyer@fedoraproject.org> wrote: > > Tested ok. Thanks. > > We've had a report of this in Fedora now. Is the above patch queued anywhere? > > https://bugzilla.redhat.com/show_bug.cgi?id=1237143 Yes, it's in tip:sched/urgent, en route to Linus. Thanks, Ingo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM 2015-07-01 6:55 ` Ingo Molnar @ 2015-07-03 13:15 ` Takashi Iwai 0 siblings, 0 replies; 19+ messages in thread From: Takashi Iwai @ 2015-07-03 13:15 UTC (permalink / raw) To: Ingo Molnar Cc: Josh Boyer, Pontus Fuchs, Peter Zijlstra, Ingo Molnar, linux-kernel, Gleb Natapov, Paolo Bonzini At Wed, 1 Jul 2015 08:55:41 +0200, Ingo Molnar wrote: > > > * Josh Boyer <jwboyer@fedoraproject.org> wrote: > > > > Tested ok. Thanks. > > > > We've had a report of this in Fedora now. Is the above patch queued anywhere? > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1237143 > > Yes, it's in tip:sched/urgent, en route to Linus. FYI, I checked the latest linux-next and indeed the warning is gone. However, the mouse cursor moves very sluggish on KVM when booted as UP. Reverting the commit 1cde2930e154 fixes it. So, the patch doesn't seem fixing all regressions. thanks, Takashi ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-07-03 15:57 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-25 12:00 Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Pontus Fuchs 2015-06-25 12:09 ` Peter Zijlstra 2015-06-25 12:15 ` Pontus Fuchs 2015-06-25 12:55 ` [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra 2015-06-30 11:10 ` [tip:sched/urgent] sched/preempt, kvm: " tip-bot for Peter Zijlstra 2015-07-03 11:23 ` Paolo Bonzini 2015-07-03 11:12 ` [PATCH] sched,kvm: " Paolo Bonzini 2015-07-03 12:19 ` Peter Zijlstra 2015-07-03 12:31 ` Paolo Bonzini 2015-07-03 13:17 ` Peter Zijlstra 2015-07-03 15:16 ` Peter Zijlstra 2015-07-03 15:26 ` Paolo Bonzini 2015-07-03 15:38 ` Paolo Bonzini 2015-07-03 15:42 ` Peter Zijlstra 2015-07-03 15:46 ` Paolo Bonzini 2015-07-03 15:57 ` Takashi Iwai 2015-06-30 13:47 ` Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM Josh Boyer 2015-07-01 6:55 ` Ingo Molnar 2015-07-03 13:15 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.