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 <[email protected]>
Date: Mon Jun 8 16:00:30 2015 +0200
sched/preempt: Add static_key() to preempt_notifiers
BR,
Pontus Fuchs
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);
}
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
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 <[email protected]>
Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers")
Reported-and-Tested-by: Pontus Fuchs <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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);
}
Commit-ID: 6efde1d3716b7d055d3310bccba2df244cf430d7
Gitweb: http://git.kernel.org/tip/6efde1d3716b7d055d3310bccba2df244cf430d7
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 25 Jun 2015 14:55:14 +0200
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Gleb Natapov <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Pontus Fuchs <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
}
On Thu, Jun 25, 2015 at 8:15 AM, Pontus Fuchs <[email protected]> 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
* Josh Boyer <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> 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);
> }
>
>
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 <[email protected]>
> AuthorDate: Thu, 25 Jun 2015 14:55:14 +0200
> Committer: Ingo Molnar <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Gleb Natapov <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Pontus Fuchs <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers")
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> 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
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?
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
At Wed, 1 Jul 2015 08:55:41 +0200,
Ingo Molnar wrote:
>
>
> * Josh Boyer <[email protected]> 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
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?
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);
}
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);
> }
>
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 <[email protected]>
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 <[email protected]>
Reported-by: Takashi Iwai <[email protected]>
Not-signed-off-by: Peter Zijlstra <[email protected]>
Not-acked-by: Peter Zijlstra <[email protected]>
Not-signed-off-by: Paolo Bonzini <[email protected]
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);
}
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 <[email protected]>
> 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 <[email protected]>
> Reported-by: Takashi Iwai <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
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 <[email protected]>
>> 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 <[email protected]>
>> Reported-by: Takashi Iwai <[email protected]>
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Ok, I'm crossing fingers and including this in my pull request in order
to preserve bisectability. Thanks.
Paolo
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 <[email protected]>
> >> 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 <[email protected]>
> >> Reported-by: Takashi Iwai <[email protected]>
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> 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 <[email protected]>
Thanks!
Takashi