[PATCH] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[PATCH] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

When starting lots of dataplane devices the bootup takes very long on my
s390 system(prototype irqfd code). With larger setups we are even able to
trigger some timeouts in some components.
Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
long (strace claims up to 0.1 sec) when having multiple CPUs.
This is caused by the  synchronize_rcu and the HZ=100 of s390.
We can defer the freeing outside of the ioctl path by using kfree_rcu.

Please note that we now have to check for a NULL pointer, since
the underlying call_rcu in kfree_rcu cannot handle broken rcu
callbacks.

This patch reduces the boot time till mounting root from 8 to 2 seconds
on my s390 guest with 100 disks.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/kvm_host.h | 1 +
 virt/kvm/irqchip.c       | 5 ++---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3d1b0e6..2f19079 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -327,6 +327,7 @@ struct kvm_kernel_irq_routing_entry {
 #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
 
 struct kvm_irq_routing_table {
+	struct rcu_head	rcu;
 	int chip[KVM_NR_IRQCHIPS][KVM_IRQCHIP_NUM_PINS];
 	struct kvm_kernel_irq_routing_entry *rt_entries;
 	u32 nr_rt_entries;
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 20dc9e4..3e2ebed 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -226,12 +226,11 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	kvm_irq_routing_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
 
-	synchronize_rcu();
-
 	new = old;
 	r = 0;
 
 out:
-	kfree(new);
+	if (new)
+		kfree_rcu(new, rcu);
 	return r;
 }
-- 
1.8.4.2

Re: [PATCH] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 16/01/2014 10:23, Christian Borntraeger ha scritto:
> When starting lots of dataplane devices the bootup takes very long on my
> s390 system(prototype irqfd code). With larger setups we are even able to
> trigger some timeouts in some components.
> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
> long (strace claims up to 0.1 sec) when having multiple CPUs.
> This is caused by the  synchronize_rcu and the HZ=100 of s390.
> We can defer the freeing outside of the ioctl path by using kfree_rcu.
> 
> Please note that we now have to check for a NULL pointer, since
> the underlying call_rcu in kfree_rcu cannot handle broken rcu
> callbacks.
> 
> This patch reduces the boot time till mounting root from 8 to 2 seconds
> on my s390 guest with 100 disks.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Hi Christian,

please take a look at the thread at
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/116933 - uses
of call_rcu need to be rate limited, otherwise the guest can allocate
arbitrarily large amounts of memory.

Can you check if using a dedicated SRCU (possibly with
synchronize_srcu_expedited in kvm_set_irq_routing) speeds up
KVM_SET_GSI_ROUTING enough?  Back-of-the-envelope, the latency should be
on the order of tens of microseconds.

Thanks,

Paolo

> ---
>  include/linux/kvm_host.h | 1 +
>  virt/kvm/irqchip.c       | 5 ++---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3d1b0e6..2f19079 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -327,6 +327,7 @@ struct kvm_kernel_irq_routing_entry {
>  #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
>  
>  struct kvm_irq_routing_table {
> +	struct rcu_head	rcu;
>  	int chip[KVM_NR_IRQCHIPS][KVM_IRQCHIP_NUM_PINS];
>  	struct kvm_kernel_irq_routing_entry *rt_entries;
>  	u32 nr_rt_entries;
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 20dc9e4..3e2ebed 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -226,12 +226,11 @@ int kvm_set_irq_routing(struct kvm *kvm,
>  	kvm_irq_routing_update(kvm, new);
>  	mutex_unlock(&kvm->irq_lock);
>  
> -	synchronize_rcu();
> -
>  	new = old;
>  	r = 0;
>  
>  out:
> -	kfree(new);
> +	if (new)
> +		kfree_rcu(new, rcu);
>  	return r;
>  }
> 

[PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

When starting lots of dataplane devices the bootup takes very long on my
s390 system(prototype irqfd code). With larger setups we are even able
to
trigger some timeouts in some components.
Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
long (strace claims up to 0.1 sec) when having multiple CPUs.
This is caused by the  synchronize_rcu and the HZ=100 of s390.
By changing the code to use a private srcu we can speed things up.

This patch reduces the boot time till mounting root from 8 to 2
seconds on my s390 guest with 100 disks.

I converted most of the rcu routines to srcu. Review for the unconverted
use of hlist_for_each_entry_rcu, hlist_add_head_rcu, hlist_del_init_rcu
is necessary, though. They look fine to me since they are protected by
outer functions.

In addition, we should also discuss if a global srcu (for all guests) is
fine.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 virt/kvm/irqchip.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 20dc9e4..5283eb8 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -26,17 +26,20 @@
 
 #include <linux/kvm_host.h>
 #include <linux/slab.h>
+#include <linux/srcu.h>
 #include <linux/export.h>
 #include <trace/events/kvm.h>
 #include "irq.h"
 
+DEFINE_STATIC_SRCU(irq_srcu);
+
 bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;
-	int gsi;
+	int gsi, idx;
 
-	rcu_read_lock();
-	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
+	idx = srcu_read_lock(&irq_srcu);
+	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
 	if (gsi != -1)
 		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
 					 link)
@@ -45,7 +48,7 @@ bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
 				return true;
 			}
 
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 
 	return false;
 }
@@ -54,18 +57,18 @@ EXPORT_SYMBOL_GPL(kvm_irq_has_notifier);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;
-	int gsi;
+	int gsi, idx;
 
 	trace_kvm_ack_irq(irqchip, pin);
 
-	rcu_read_lock();
-	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
+	idx = srcu_read_lock(&irq_srcu);
+	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
 	if (gsi != -1)
 		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
 					 link)
 			if (kian->gsi == gsi)
 				kian->irq_acked(kian);
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 }
 
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
@@ -85,7 +88,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	hlist_del_init_rcu(&kian->link);
 	mutex_unlock(&kvm->irq_lock);
-	synchronize_rcu();
+	synchronize_srcu_expedited(&irq_srcu);
 #ifdef __KVM_HAVE_IOAPIC
 	kvm_vcpu_request_scan_ioapic(kvm);
 #endif
@@ -115,7 +118,7 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
 		bool line_status)
 {
 	struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
-	int ret = -1, i = 0;
+	int ret = -1, i = 0, idx;
 	struct kvm_irq_routing_table *irq_rt;
 
 	trace_kvm_set_irq(irq, level, irq_source_id);
@@ -124,12 +127,12 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	rcu_read_lock();
-	irq_rt = rcu_dereference(kvm->irq_routing);
+	idx = srcu_read_lock(&irq_srcu);
+	irq_rt = srcu_dereference(kvm->irq_routing, &irq_srcu);
 	if (irq < irq_rt->nr_rt_entries)
 		hlist_for_each_entry(e, &irq_rt->map[irq], link)
 			irq_set[i++] = *e;
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 
 	while(i--) {
 		int r;
@@ -226,7 +229,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	kvm_irq_routing_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
 
-	synchronize_rcu();
+	synchronize_srcu_expedited(&irq_srcu);
 
 	new = old;
 	r = 0;
-- 
1.8.4.2

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 16/01/2014 13:44, Christian Borntraeger ha scritto:
> I converted most of the rcu routines to srcu. Review for the unconverted
> use of hlist_for_each_entry_rcu, hlist_add_head_rcu, hlist_del_init_rcu
> is necessary, though. They look fine to me since they are protected by
> outer functions.

They are fine because they do not have lockdep checks
(hlist_for_each_entry_rcu uses rcu_dereference_raw rather than
rcu_dereference, and write-sides do not do rcu lockdep at all).

> In addition, we should also discuss if a global srcu (for all guests) is
> fine.

I think it is.  It is already way cheaper than it used to be, and we're
hardly relying on the "sleepable" part of srcu.  We just want its faster
detection of grace periods.  One instance should be fine because our
read sides are so small and mostly they are not even preemptable.

Thanks for writing the patch!

Paolo

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

On 16/01/14 13:59, Paolo Bonzini wrote:
> Il 16/01/2014 13:44, Christian Borntraeger ha scritto:
>> I converted most of the rcu routines to srcu. Review for the unconverted
>> use of hlist_for_each_entry_rcu, hlist_add_head_rcu, hlist_del_init_rcu
>> is necessary, though. They look fine to me since they are protected by
>> outer functions.
> 
> They are fine because they do not have lockdep checks
> (hlist_for_each_entry_rcu uses rcu_dereference_raw rather than
> rcu_dereference, and write-sides do not do rcu lockdep at all).
> 
>> In addition, we should also discuss if a global srcu (for all guests) is
>> fine.
> 
> I think it is.  It is already way cheaper than it used to be, and we're
> hardly relying on the "sleepable" part of srcu.  We just want its faster
> detection of grace periods.  One instance should be fine because our
> read sides are so small and mostly they are not even preemptable.
> 
> Thanks for writing the patch!
> 
> Paolo
> 

Will you edit the patch description or shall I resend the patch?

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 16/01/2014 14:06, Christian Borntraeger ha scritto:
> Will you edit the patch description or shall I resend the patch?

I can edit the commit message.

Paolo

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Michael S. Tsirkin]

On Thu, Jan 16, 2014 at 02:07:19PM +0100, Paolo Bonzini wrote:
> Il 16/01/2014 14:06, Christian Borntraeger ha scritto:
> > Will you edit the patch description or shall I resend the patch?
> 
> I can edit the commit message.
> 
> Paolo

I think we really need to see the effect adding srcu has on interrupt
injection.

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

On 16/01/14 19:56, Michael S. Tsirkin wrote:
> On Thu, Jan 16, 2014 at 02:07:19PM +0100, Paolo Bonzini wrote:
>> Il 16/01/2014 14:06, Christian Borntraeger ha scritto:
>>> Will you edit the patch description or shall I resend the patch?
>>
>> I can edit the commit message.
>>
>> Paolo
> 
> I think we really need to see the effect adding srcu has on interrupt
> injection.

Michael, 
do you have a quick way to check if srcu has a noticeable impact on int
injection on your systems? I am happy with either v2 or v3 of the patch,
but srcu_synchronize_expedited seems to have less latency impact on the 
full system than rcu_synchronize_expedited. This might give Paolo a hint
which of the patches is the right way to go.

Christian

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 17/01/2014 09:29, Christian Borntraeger ha scritto:
> Michael, 
> do you have a quick way to check if srcu has a noticeable impact on int
> injection on your systems? I am happy with either v2 or v3 of the patch,
> but srcu_synchronize_expedited seems to have less latency impact on the 
> full system than rcu_synchronize_expedited. This might give Paolo a hint
> which of the patches is the right way to go.

For 3.14, I'll definitely pick v3.

Paolo

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 17/01/2014 09:29, Christian Borntraeger ha scritto:
> Michael,
> do you have a quick way to check if srcu has a noticeable impact on int
> injection on your systems? I am happy with either v2 or v3 of the patch,
> but srcu_synchronize_expedited seems to have less latency impact on the
> full system than rcu_synchronize_expedited. This might give Paolo a hint
> which of the patches is the right way to go.

Hi all,

I've asked Andrew Theurer to run network tests on a 10G connection (TCP 
request/response to check for performance, TCP streaming for host CPU 
utilization).

Paolo

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Andrew Theurer]

> Il 17/01/2014 09:29, Christian Borntraeger ha scritto:
> > Michael,
> > do you have a quick way to check if srcu has a noticeable impact on int
> > injection on your systems? I am happy with either v2 or v3 of the patch,
> > but srcu_synchronize_expedited seems to have less latency impact on the
> > full system than rcu_synchronize_expedited. This might give Paolo a hint
> > which of the patches is the right way to go.
> 
> Hi all,
> 
> I've asked Andrew Theurer to run network tests on a 10G connection (TCP
> request/response to check for performance, TCP streaming for host CPU
> utilization).

I am hoping to have some results some time tomorrow (Friday).

-Andrew

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Michael S. Tsirkin]

On Thu, Jan 16, 2014 at 01:44:20PM +0100, Christian Borntraeger wrote:
> When starting lots of dataplane devices the bootup takes very long on my
> s390 system(prototype irqfd code). With larger setups we are even able
> to
> trigger some timeouts in some components.
> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
> long (strace claims up to 0.1 sec) when having multiple CPUs.
> This is caused by the  synchronize_rcu and the HZ=100 of s390.
> By changing the code to use a private srcu we can speed things up.
> 
> This patch reduces the boot time till mounting root from 8 to 2
> seconds on my s390 guest with 100 disks.
> 
> I converted most of the rcu routines to srcu. Review for the unconverted
> use of hlist_for_each_entry_rcu, hlist_add_head_rcu, hlist_del_init_rcu
> is necessary, though. They look fine to me since they are protected by
> outer functions.
> 
> In addition, we should also discuss if a global srcu (for all guests) is
> fine.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>


That's nice but did you try to measure the overhead
on some interrupt-intensive workloads, such as RX with 10G ethernet?
srcu locks aren't free like rcu ones.

> ---
>  virt/kvm/irqchip.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 20dc9e4..5283eb8 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -26,17 +26,20 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/slab.h>
> +#include <linux/srcu.h>
>  #include <linux/export.h>
>  #include <trace/events/kvm.h>
>  #include "irq.h"
>  
> +DEFINE_STATIC_SRCU(irq_srcu);
> +
>  bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
>  {
>  	struct kvm_irq_ack_notifier *kian;
> -	int gsi;
> +	int gsi, idx;
>  
> -	rcu_read_lock();
> -	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
> +	idx = srcu_read_lock(&irq_srcu);
> +	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
>  	if (gsi != -1)
>  		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
>  					 link)
> @@ -45,7 +48,7 @@ bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
>  				return true;
>  			}
>  
> -	rcu_read_unlock();
> +	srcu_read_unlock(&irq_srcu, idx);
>  
>  	return false;
>  }
> @@ -54,18 +57,18 @@ EXPORT_SYMBOL_GPL(kvm_irq_has_notifier);
>  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
>  {
>  	struct kvm_irq_ack_notifier *kian;
> -	int gsi;
> +	int gsi, idx;
>  
>  	trace_kvm_ack_irq(irqchip, pin);
>  
> -	rcu_read_lock();
> -	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
> +	idx = srcu_read_lock(&irq_srcu);
> +	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
>  	if (gsi != -1)
>  		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
>  					 link)
>  			if (kian->gsi == gsi)
>  				kian->irq_acked(kian);
> -	rcu_read_unlock();
> +	srcu_read_unlock(&irq_srcu, idx);
>  }
>  
>  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> @@ -85,7 +88,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
>  	mutex_lock(&kvm->irq_lock);
>  	hlist_del_init_rcu(&kian->link);
>  	mutex_unlock(&kvm->irq_lock);
> -	synchronize_rcu();
> +	synchronize_srcu_expedited(&irq_srcu);
>  #ifdef __KVM_HAVE_IOAPIC
>  	kvm_vcpu_request_scan_ioapic(kvm);
>  #endif
> @@ -115,7 +118,7 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
>  		bool line_status)
>  {
>  	struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
> -	int ret = -1, i = 0;
> +	int ret = -1, i = 0, idx;
>  	struct kvm_irq_routing_table *irq_rt;
>  
>  	trace_kvm_set_irq(irq, level, irq_source_id);
> @@ -124,12 +127,12 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
>  	 * IOAPIC.  So set the bit in both. The guest will ignore
>  	 * writes to the unused one.
>  	 */
> -	rcu_read_lock();
> -	irq_rt = rcu_dereference(kvm->irq_routing);
> +	idx = srcu_read_lock(&irq_srcu);
> +	irq_rt = srcu_dereference(kvm->irq_routing, &irq_srcu);
>  	if (irq < irq_rt->nr_rt_entries)
>  		hlist_for_each_entry(e, &irq_rt->map[irq], link)
>  			irq_set[i++] = *e;
> -	rcu_read_unlock();
> +	srcu_read_unlock(&irq_srcu, idx);
>  
>  	while(i--) {
>  		int r;
> @@ -226,7 +229,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
>  	kvm_irq_routing_update(kvm, new);
>  	mutex_unlock(&kvm->irq_lock);
>  
> -	synchronize_rcu();
> +	synchronize_srcu_expedited(&irq_srcu);

Hmm, it's a bit strange that you also do _expecited here.
What if this synchronize_rcu is replaced by synchronize_rcu_expedited
and no other changes are made?
Maybe that's enough?

>  
>  	new = old;
>  	r = 0;
> -- 
> 1.8.4.2

[PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

On 16/01/14 19:55, Michael S. Tsirkin wrote:
> On Thu, Jan 16, 2014 at 01:44:20PM +0100, Christian Borntraeger wrote:
[...]
>> I converted most of the rcu routines to srcu. Review for the unconverted
[...]

> That's nice but did you try to measure the overhead
> on some interrupt-intensive workloads, such as RX with 10G ethernet?
> srcu locks aren't free like rcu ones.

Right, but the read side is only acting on cpu local data structures so the overhead
is probably very small in contrast to the hlist work and the injection itself.
You have a more compelling review comment, though:

[...]
>> -	synchronize_rcu();
>> +	synchronize_srcu_expedited(&irq_srcu);
> 
> Hmm, it's a bit strange that you also do _expecited here.

Well, I just did what the original mail thread suggested :-)

> What if this synchronize_rcu is replaced by synchronize_rcu_expedited
> and no other changes are made?
> Maybe that's enough?

Yes, its enough. (seems slightly slower than v2, but fast enough)

Patch below:


[PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

When starting lots of dataplane devices the bootup takes very long
on my s390 system(prototype irqfd code). With larger setups we are even
able to trigger some timeouts in some components.
Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
long (strace claims up to 0.1 sec) when having multiple CPUs.
This is caused by the  synchronize_rcu and the HZ=100 of s390.

Lets use the expedited variant to speed things up as suggested by
Michael S. Tsirkin

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 virt/kvm/irqchip.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 20dc9e4..dbcfde7 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -226,7 +226,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	kvm_irq_routing_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
 
-	synchronize_rcu();
+	synchronize_rcu_expedited();
 
 	new = old;
 	r = 0;

Re: [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Michael S. Tsirkin]

On Thu, Jan 16, 2014 at 09:07:14PM +0100, Christian Borntraeger wrote:
> On 16/01/14 19:55, Michael S. Tsirkin wrote:
> > On Thu, Jan 16, 2014 at 01:44:20PM +0100, Christian Borntraeger wrote:
> [...]
> >> I converted most of the rcu routines to srcu. Review for the unconverted
> [...]
> 
> > That's nice but did you try to measure the overhead
> > on some interrupt-intensive workloads, such as RX with 10G ethernet?
> > srcu locks aren't free like rcu ones.
> 
> Right, but the read side is only acting on cpu local data structures so the overhead
> is probably very small in contrast to the hlist work and the injection itself.

My testing of VM exit paths shows the overhead of read size RCU
is not negligeable, IIRC it involves memory barriers which
are expensive operations.

> You have a more compelling review comment, though:
> 
> [...]
> >> -	synchronize_rcu();
> >> +	synchronize_srcu_expedited(&irq_srcu);
> > 
> > Hmm, it's a bit strange that you also do _expecited here.
> 
> Well, I just did what the original mail thread suggested :-)
> 
> > What if this synchronize_rcu is replaced by synchronize_rcu_expedited
> > and no other changes are made?
> > Maybe that's enough?
> 
> Yes, its enough. (seems slightly slower than v2, but fast enough)
> 
> Patch below:
> 
> 
> [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING
> 
> When starting lots of dataplane devices the bootup takes very long
> on my s390 system(prototype irqfd code). With larger setups we are even
> able to trigger some timeouts in some components.
> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
> long (strace claims up to 0.1 sec) when having multiple CPUs.
> This is caused by the  synchronize_rcu and the HZ=100 of s390.
> 
> Lets use the expedited variant to speed things up as suggested by
> Michael S. Tsirkin
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  virt/kvm/irqchip.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 20dc9e4..dbcfde7 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -226,7 +226,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
>  	kvm_irq_routing_update(kvm, new);
>  	mutex_unlock(&kvm->irq_lock);
>  
> -	synchronize_rcu();
> +	synchronize_rcu_expedited();
>  
>  	new = old;
>  	r = 0;

Re: [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 16/01/2014 21:22, Michael S. Tsirkin ha scritto:
>> [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING
>>
>> When starting lots of dataplane devices the bootup takes very long
>> on my s390 system(prototype irqfd code). With larger setups we are even
>> able to trigger some timeouts in some components.
>> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
>> long (strace claims up to 0.1 sec) when having multiple CPUs.
>> This is caused by the  synchronize_rcu and the HZ=100 of s390.
>>
>> Lets use the expedited variant to speed things up as suggested by
>> Michael S. Tsirkin
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  virt/kvm/irqchip.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
>> index 20dc9e4..dbcfde7 100644
>> --- a/virt/kvm/irqchip.c
>> +++ b/virt/kvm/irqchip.c
>> @@ -226,7 +226,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
>>  	kvm_irq_routing_update(kvm, new);
>>  	mutex_unlock(&kvm->irq_lock);
>>  
>> -	synchronize_rcu();
>> +	synchronize_rcu_expedited();
>>  
>>  	new = old;
>>  	r = 0;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Well... I love to contradict myself, so: no way this can be accepted
this close to the end of the merge window.  :(

synchronize_rcu_expedited() forces a context switch on all CPUs, even
those that are not running KVM.  Thus, this patch might help a guest DoS
its host by changing the IRQ routing tables in a loop.

So this will have to wait for 3.15.  We have ~2 months to do performance
measurements on the v2 patch.  Sorry.

Thanks,

Paolo

Re: [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

On 17/01/14 15:03, Paolo Bonzini wrote:
> Il 16/01/2014 21:22, Michael S. Tsirkin ha scritto:
>>> [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING
>>>
>>> When starting lots of dataplane devices the bootup takes very long
>>> on my s390 system(prototype irqfd code). With larger setups we are even
>>> able to trigger some timeouts in some components.
>>> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
>>> long (strace claims up to 0.1 sec) when having multiple CPUs.
>>> This is caused by the  synchronize_rcu and the HZ=100 of s390.
>>>
>>> Lets use the expedited variant to speed things up as suggested by
>>> Michael S. Tsirkin
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  virt/kvm/irqchip.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
>>> index 20dc9e4..dbcfde7 100644
>>> --- a/virt/kvm/irqchip.c
>>> +++ b/virt/kvm/irqchip.c
>>> @@ -226,7 +226,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
>>>  	kvm_irq_routing_update(kvm, new);
>>>  	mutex_unlock(&kvm->irq_lock);
>>>  
>>> -	synchronize_rcu();
>>> +	synchronize_rcu_expedited();
>>>  
>>>  	new = old;
>>>  	r = 0;
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> Well... I love to contradict myself, so: no way this can be accepted
> this close to the end of the merge window.  :(
> 
> synchronize_rcu_expedited() forces a context switch on all CPUs, even
> those that are not running KVM.  Thus, this patch might help a guest DoS
> its host by changing the IRQ routing tables in a loop.
> 
> So this will have to wait for 3.15.  We have ~2 months to do performance
> measurements on the v2 patch.  Sorry.

Any chance that you or Michael can give some performance feedback on v2? All
my lab systems are s390 and not x86...

Re: [PATCHv3] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 17/01/2014 16:03, Christian Borntraeger ha scritto:
> > Well... I love to contradict myself, so: no way this can be accepted
> > this close to the end of the merge window.  :(
> > 
> > synchronize_rcu_expedited() forces a context switch on all CPUs, even
> > those that are not running KVM.  Thus, this patch might help a guest DoS
> > its host by changing the IRQ routing tables in a loop.
> > 
> > So this will have to wait for 3.15.  We have ~2 months to do performance
> > measurements on the v2 patch.  Sorry.
> 
> Any chance that you or Michael can give some performance feedback on v2? All
> my lab systems are s390 and not x86...

Yes, we will help.

Paolo

Re: [PATCHv2/RFC] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 16/01/2014 13:44, Christian Borntraeger ha scritto:
> +DEFINE_STATIC_SRCU(irq_srcu);
> +
>  bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
>  {
>  	struct kvm_irq_ack_notifier *kian;
> -	int gsi;
> +	int gsi, idx;
>  
> -	rcu_read_lock();
> -	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
> +	idx = srcu_read_lock(&irq_srcu);
> +	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
>  	if (gsi != -1)
>  		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
>  					 link)
> @@ -45,7 +48,7 @@ bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
>  				return true;
>  			}
>  
> -	rcu_read_unlock();
> +	srcu_read_unlock(&irq_srcu, idx);

Missing hunk here:

@@ -44,7 +44,7 @@ bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
 		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
 					 link)
 			if (kian->gsi == gsi) {
-				rcu_read_unlock();
+				srcu_read_unlock(&irq_srcu, idx);
 				return true;
 			}
 

Paolo

[PATCHv2] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Christian Borntraeger]

When starting lots of dataplane devices the bootup takes very long on my
s390 system(prototype irqfd code). With larger setups we are even able
to trigger some timeouts in some userspace components.
Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
long (strace claims up to 0.1 sec) when having multiple CPUs.
This is caused by the  synchronize_rcu and the HZ=100 of s390.
By changing the code to use a private srcu we can speed things up.

This patch reduces the boot time till mounting root from 8 to 2
seconds on my s390 guest with 100 disks.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
v1-v2: Fix missing hunk in kvm_irq_has_notifier. This was unnoticed on
       s390, as our code did not use it.
---
 virt/kvm/irqchip.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 20dc9e4..7598f5a 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -26,26 +26,29 @@
 
 #include <linux/kvm_host.h>
 #include <linux/slab.h>
+#include <linux/srcu.h>
 #include <linux/export.h>
 #include <trace/events/kvm.h>
 #include "irq.h"
 
+DEFINE_STATIC_SRCU(irq_srcu);
+
 bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;
-	int gsi;
+	int gsi, idx;
 
-	rcu_read_lock();
-	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
+	idx = srcu_read_lock(&irq_srcu);
+	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
 	if (gsi != -1)
 		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
 					 link)
 			if (kian->gsi == gsi) {
-				rcu_read_unlock();
+				srcu_read_unlock(&irq_srcu, idx);
 				return true;
 			}
 
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 
 	return false;
 }
@@ -54,18 +57,18 @@ EXPORT_SYMBOL_GPL(kvm_irq_has_notifier);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;
-	int gsi;
+	int gsi, idx;
 
 	trace_kvm_ack_irq(irqchip, pin);
 
-	rcu_read_lock();
-	gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
+	idx = srcu_read_lock(&irq_srcu);
+	gsi = srcu_dereference(kvm->irq_routing, &irq_srcu)->chip[irqchip][pin];
 	if (gsi != -1)
 		hlist_for_each_entry_rcu(kian, &kvm->irq_ack_notifier_list,
 					 link)
 			if (kian->gsi == gsi)
 				kian->irq_acked(kian);
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 }
 
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
@@ -85,7 +88,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	hlist_del_init_rcu(&kian->link);
 	mutex_unlock(&kvm->irq_lock);
-	synchronize_rcu();
+	synchronize_srcu_expedited(&irq_srcu);
 #ifdef __KVM_HAVE_IOAPIC
 	kvm_vcpu_request_scan_ioapic(kvm);
 #endif
@@ -115,7 +118,7 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
 		bool line_status)
 {
 	struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
-	int ret = -1, i = 0;
+	int ret = -1, i = 0, idx;
 	struct kvm_irq_routing_table *irq_rt;
 
 	trace_kvm_set_irq(irq, level, irq_source_id);
@@ -124,12 +127,12 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	rcu_read_lock();
-	irq_rt = rcu_dereference(kvm->irq_routing);
+	idx = srcu_read_lock(&irq_srcu);
+	irq_rt = srcu_dereference(kvm->irq_routing, &irq_srcu);
 	if (irq < irq_rt->nr_rt_entries)
 		hlist_for_each_entry(e, &irq_rt->map[irq], link)
 			irq_set[i++] = *e;
-	rcu_read_unlock();
+	srcu_read_unlock(&irq_srcu, idx);
 
 	while(i--) {
 		int r;
@@ -226,7 +229,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	kvm_irq_routing_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
 
-	synchronize_rcu();
+	synchronize_srcu_expedited(&irq_srcu);
 
 	new = old;
 	r = 0;
-- 
1.8.4.2

Re: [PATCHv2] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 24/02/2014 12:58, Christian Borntraeger ha scritto:
> When starting lots of dataplane devices the bootup takes very long on my
> s390 system(prototype irqfd code). With larger setups we are even able
> to trigger some timeouts in some userspace components.
> Turns out that the KVM_SET_GSI_ROUTING ioctl takes very
> long (strace claims up to 0.1 sec) when having multiple CPUs.
> This is caused by the  synchronize_rcu and the HZ=100 of s390.
> By changing the code to use a private srcu we can speed things up.
>
> This patch reduces the boot time till mounting root from 8 to 2
> seconds on my s390 guest with 100 disks.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> ---
> v1-v2: Fix missing hunk in kvm_irq_has_notifier. This was unnoticed on
>        s390, as our code did not use it.

In fact, there are other accesses to irq_routing elsewhere in virt/kvm 
which should be changed to irq_srcu (which in turn probably means that 
it's better to make the SRCU instance per-VM).

My fault, I should have delayed the patch to after the merge window 
instead of doing a shoddy review.  I'll test the complete patch myself 
since VFIO is probably the only thing that can cover it 100%, and s390 
doesn't do VFIO.

Paolo