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


Groups > linux.kernel > #1176569 > unrolled thread

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

Started byPeter Zijlstra <peterz@infradead.org>
First post2015-07-03 15:20 +0200
Last post2015-07-03 18:00 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra <peterz@infradead.org> - 2015-07-03 15:20 +0200
    Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra <peterz@infradead.org> - 2015-07-03 17:20 +0200
      Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Paolo Bonzini <pbonzini@redhat.com> - 2015-07-03 17:30 +0200
        Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Paolo Bonzini <pbonzini@redhat.com> - 2015-07-03 17:40 +0200
          Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Peter Zijlstra <peterz@infradead.org> - 2015-07-03 17:50 +0200
            Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Paolo Bonzini <pbonzini@redhat.com> - 2015-07-03 17:50 +0200
              Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage Takashi Iwai <tiwai@suse.de> - 2015-07-03 18:00 +0200

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

FromPeter Zijlstra <peterz@infradead.org>
Date2015-07-03 15:20 +0200
SubjectRe: [PATCH] sched,kvm: Fix KVM preempt_notifier usage
Message-ID<pI92O-xt-15@gated-at.bofh.it>
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1176653

FromPeter Zijlstra <peterz@infradead.org>
Date2015-07-03 17:20 +0200
Message-ID<pIaUW-1GA-15@gated-at.bofh.it>
In reply to#1176569
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);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1176668

FromPaolo Bonzini <pbonzini@redhat.com>
Date2015-07-03 17:30 +0200
Message-ID<pIb4D-1JT-29@gated-at.bofh.it>
In reply to#1176653

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);
>  }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1176679

FromPaolo Bonzini <pbonzini@redhat.com>
Date2015-07-03 17:40 +0200
Message-ID<pIbei-1No-29@gated-at.bofh.it>
In reply to#1176668

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1176682

FromPeter Zijlstra <peterz@infradead.org>
Date2015-07-03 17:50 +0200
Message-ID<pIbnX-1T7-15@gated-at.bofh.it>
In reply to#1176679
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>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1176683

FromPaolo Bonzini <pbonzini@redhat.com>
Date2015-07-03 17:50 +0200
Message-ID<pIbnX-1T7-17@gated-at.bofh.it>
In reply to#1176682

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1176685

FromTakashi Iwai <tiwai@suse.de>
Date2015-07-03 18:00 +0200
Message-ID<pIbxD-1Wv-3@gated-at.bofh.it>
In reply to#1176683
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web