Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Pontus Fuchs]

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

Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Peter Zijlstra]

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);
 }
 

Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Pontus Fuchs]

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

[PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Peter Zijlstra]

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(&notifier->link, &current->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(&notifier->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);
 }
 

[tip:sched/urgent] sched/preempt, kvm: Fix KVM preempt_notifier usage

[tip-bot for Peter Zijlstra]

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(&notifier->link, &current->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(&notifier->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);
 }
 

Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Josh Boyer]

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

Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Ingo Molnar]

* 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

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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(&notifier->link, &current->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(&notifier->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);
>  }
>  
> 

Re: [tip:sched/urgent] sched/preempt, kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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(&notifier->link, &current->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(&notifier->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

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Peter Zijlstra]

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?

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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

Re: Regression: sched/preempt: Add static_key() to preempt_notifiers breaks my KVM

[Takashi Iwai]

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

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Peter Zijlstra]

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?

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Peter Zijlstra]

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(&notifier->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);
 }

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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(&notifier->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);
>  }
> 

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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(&notifier->link, &current->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(&notifier->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);
 }

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Peter Zijlstra]

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>

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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

Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Takashi Iwai]

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