[v2 0/4] KVM: irqchip: synchronize srcu only if needed

[v2 0/4] KVM: irqchip: synchronize srcu only if needed

[Yi Wang]

From: Yi Wang <foxywang@tencent.com>

We found that it may cost more than 20 milliseconds very accidentally
to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
already.

The reason is that when vmm(qemu/CloudHypervisor) invokes
KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
might_sleep and kworker of srcu may cost some delay during this period.
One way makes sence is setup empty irq routing when creating vm and
so that x86/s390 don't need to setup empty/dummy irq routing.

Note: I have no s390 machine so the s390 patch has not been tested.

Changelog:
----------
v1->v2:
  - setup empty irq routing in kvm_create_vm
  - don't setup irq routing in x86 KVM_CAP_SPLIT_IRQCHIP
  - don't setup irq routing in s390 KVM_CREATE_IRQCHIP

v1: https://lore.kernel.org/kvm/20240112091128.3868059-1-foxywang@tencent.com/

Yi Wang (4):
  KVM: irqchip: add setup empty irq routing function
  KVM: setup empty irq routing when create vm
  KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP
  KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

 arch/s390/kvm/kvm-s390.c | 13 +++++--------
 arch/x86/kvm/irq.h       |  1 -
 arch/x86/kvm/irq_comm.c  |  5 -----
 arch/x86/kvm/x86.c       |  3 ---
 include/linux/kvm_host.h |  1 +
 virt/kvm/irqchip.c       | 19 +++++++++++++++++++
 virt/kvm/kvm_main.c      |  4 ++++
 7 files changed, 29 insertions(+), 17 deletions(-)

-- 
2.39.3

[v2 1/4] KVM: irqchip: add setup empty irq routing function

[Yi Wang]

Add a new function to setup empty irq routing in kvm path, which
can be invoded in non-architecture-specific functions. The difference
compared to the kvm_setup_empty_irq_routing() is this function just
alloc the empty irq routing and does not need synchronize srcu, as
we will call it in kvm_create_vm().

This patch is a preparatory step for an upcoming patch to avoid
delay in KVM_CAP_SPLIT_IRQCHIP ioctl.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 include/linux/kvm_host.h |  1 +
 virt/kvm/irqchip.c       | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4944136efaa2..e91525c0a4ea 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2000,6 +2000,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
 			const struct kvm_irq_routing_entry *entries,
 			unsigned nr,
 			unsigned flags);
+int kvm_setup_empty_irq_routing_lockless(struct kvm *kvm);
 int kvm_set_routing_entry(struct kvm *kvm,
 			  struct kvm_kernel_irq_routing_entry *e,
 			  const struct kvm_irq_routing_entry *ue);
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 1e567d1f6d3d..90fc43bd0fe4 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -237,3 +237,22 @@ int kvm_set_irq_routing(struct kvm *kvm,
 
 	return r;
 }
+
+int kvm_setup_empty_irq_routing_lockless(struct kvm *kvm)
+{
+	struct kvm_irq_routing_table *new;
+	u32 i, j;
+
+	new = kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
+	if (!new)
+		return -ENOMEM;
+
+	new->nr_rt_entries = 1;
+	for (i = 0; i < KVM_NR_IRQCHIPS; i++)
+		for (j = 0; j < KVM_IRQCHIP_NUM_PINS; j++)
+			new->chip[i][j] = -1;
+
+	RCU_INIT_POINTER(kvm->irq_routing, new);
+
+	return 0;
+}
-- 
2.39.3

Re: [v2 1/4] KVM: irqchip: add setup empty irq routing function

[Oliver Upton]

On Sun, Jan 21, 2024 at 07:17:27PM +0800, Yi Wang wrote:
> Add a new function to setup empty irq routing in kvm path, which
> can be invoded in non-architecture-specific functions. The difference
> compared to the kvm_setup_empty_irq_routing() is this function just
> alloc the empty irq routing and does not need synchronize srcu, as
> we will call it in kvm_create_vm().
> 
> This patch is a preparatory step for an upcoming patch to avoid
> delay in KVM_CAP_SPLIT_IRQCHIP ioctl.

Adding a function in a separate patch from its callsites is never
useful. Please squash this into the second patch.

-- 
Thanks,
Oliver

Re: [v2 1/4] KVM: irqchip: add setup empty irq routing function

[Yi Wang]

On Wed, Jan 24, 2024 at 2:47 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Sun, Jan 21, 2024 at 07:17:27PM +0800, Yi Wang wrote:
> > Add a new function to setup empty irq routing in kvm path, which
> > can be invoded in non-architecture-specific functions. The difference
> > compared to the kvm_setup_empty_irq_routing() is this function just
> > alloc the empty irq routing and does not need synchronize srcu, as
> > we will call it in kvm_create_vm().
> >
> > This patch is a preparatory step for an upcoming patch to avoid
> > delay in KVM_CAP_SPLIT_IRQCHIP ioctl.
>
> Adding a function in a separate patch from its callsites is never
> useful. Please squash this into the second patch.

Thanks for your review and suggestion. I will update this patch ASAP.

>
> --
> Thanks,
> Oliver



-- 
---
Best wishes
Yi Wang

[v2 2/4] KVM: setup empty irq routing when create vm

[Yi Wang]

Setup empty irq routing when kvm_create_vm(), so that x86 and s390
no longer need to set empty/dummy irq routing when creating an
IRQCHIP 'cause it avoid an synchronize_srcu.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 virt/kvm/kvm_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7db96875ac46..db1b13fc0502 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1242,6 +1242,10 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
 	if (r)
 		goto out_err;
 
+	r = kvm_setup_empty_irq_routing_lockless(kvm);
+	if (r)
+		goto out_err;
+
 	mutex_lock(&kvm_lock);
 	list_add(&kvm->vm_list, &vm_list);
 	mutex_unlock(&kvm_lock);
-- 
2.39.3

[v2 3/4] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP

[Yi Wang]

We found that it may cost more than 20 milliseconds very accidentally
to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
already.

The reason is that when vmm(qemu/CloudHypervisor) invokes
KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
might_sleep and kworker of srcu may cost some delay during this period.

As we have set up empty irq routing when creating vm, so this is no
need now.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 arch/x86/kvm/irq.h      | 1 -
 arch/x86/kvm/irq_comm.c | 5 -----
 arch/x86/kvm/x86.c      | 3 ---
 3 files changed, 9 deletions(-)

diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index c2d7cfe82d00..76d46b2f41dd 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -106,7 +106,6 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
 int apic_has_pending_timer(struct kvm_vcpu *vcpu);
 
 int kvm_setup_default_irq_routing(struct kvm *kvm);
-int kvm_setup_empty_irq_routing(struct kvm *kvm);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			     struct kvm_lapic_irq *irq,
 			     struct dest_map *dest_map);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 16d076a1b91a..99bf53b94175 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -392,11 +392,6 @@ int kvm_setup_default_irq_routing(struct kvm *kvm)
 
 static const struct kvm_irq_routing_entry empty_routing[] = {};
 
-int kvm_setup_empty_irq_routing(struct kvm *kvm)
-{
-	return kvm_set_irq_routing(kvm, empty_routing, 0, 0);
-}
-
 void kvm_arch_post_irq_routing_update(struct kvm *kvm)
 {
 	if (!irqchip_split(kvm))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cec0fc2a4b1c..6a2e786aca22 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6414,9 +6414,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			goto split_irqchip_unlock;
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
-		r = kvm_setup_empty_irq_routing(kvm);
-		if (r)
-			goto split_irqchip_unlock;
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
-- 
2.39.3

[v2 4/4] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Yi Wang]

As we have setup empty irq routing in kvm_create_vm(), there's
no need to setup dummy routing when KVM_CREATE_IRQCHIP.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 arch/s390/kvm/kvm-s390.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index acc81ca6492e..7c836c973b75 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2999,14 +2999,11 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		break;
 	}
 	case KVM_CREATE_IRQCHIP: {
-		struct kvm_irq_routing_entry routing;
-
-		r = -EINVAL;
-		if (kvm->arch.use_irqchip) {
-			/* Set up dummy routing. */
-			memset(&routing, 0, sizeof(routing));
-			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
-		}
+		/*
+		 * As we have set up empty routing, there is no need to
+		 * setup dummy routing here.
+		 */
+		r = 0;
 		break;
 	}
 	case KVM_SET_DEVICE_ATTR: {
-- 
2.39.3

Re: [v2 4/4] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Oliver Upton]

On Sun, Jan 21, 2024 at 07:17:30PM +0800, Yi Wang wrote:
> As we have setup empty irq routing in kvm_create_vm(), there's
> no need to setup dummy routing when KVM_CREATE_IRQCHIP.
> 
> Signed-off-by: Yi Wang <foxywang@tencent.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index acc81ca6492e..7c836c973b75 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2999,14 +2999,11 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		break;
>  	}
>  	case KVM_CREATE_IRQCHIP: {
> -		struct kvm_irq_routing_entry routing;
> -
> -		r = -EINVAL;
> -		if (kvm->arch.use_irqchip) {
> -			/* Set up dummy routing. */
> -			memset(&routing, 0, sizeof(routing));
> -			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
> -		}
> +		/*
> +		 * As we have set up empty routing, there is no need to
> +		 * setup dummy routing here.
> +		 */

Where exactly?

In the context of this patch series it is rather obvious, but this
comment does not stand on its own. You can either throw the reader a
bone by mentioning where the dummy routing is created or just drop the
comment altogether.

> +		r = 0;
>  		break;
>  	}
>  	case KVM_SET_DEVICE_ATTR: {
> -- 
> 2.39.3
> 

-- 
Thanks,
Oliver

Re: [v2 4/4] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Yi Wang]

On Wed, Jan 24, 2024 at 2:54 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Sun, Jan 21, 2024 at 07:17:30PM +0800, Yi Wang wrote:
> > As we have setup empty irq routing in kvm_create_vm(), there's
> > no need to setup dummy routing when KVM_CREATE_IRQCHIP.
> >
> > Signed-off-by: Yi Wang <foxywang@tencent.com>
> > ---
> >  arch/s390/kvm/kvm-s390.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index acc81ca6492e..7c836c973b75 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2999,14 +2999,11 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >               break;
> >       }
> >       case KVM_CREATE_IRQCHIP: {
> > -             struct kvm_irq_routing_entry routing;
> > -
> > -             r = -EINVAL;
> > -             if (kvm->arch.use_irqchip) {
> > -                     /* Set up dummy routing. */
> > -                     memset(&routing, 0, sizeof(routing));
> > -                     r = kvm_set_irq_routing(kvm, &routing, 0, 0);
> > -             }
> > +             /*
> > +              * As we have set up empty routing, there is no need to
> > +              * setup dummy routing here.
> > +              */
>
> Where exactly?
>
> In the context of this patch series it is rather obvious, but this
> comment does not stand on its own. You can either throw the reader a
> bone by mentioning where the dummy routing is created or just drop the
> comment altogether.

Yeap, you are right. I will drop this in the upcoming patch.

>
> > +             r = 0;
> >               break;
> >       }
> >       case KVM_SET_DEVICE_ATTR: {
> > --
> > 2.39.3
> >
>
> --
> Thanks,
> Oliver



-- 
---
Best wishes
Yi Wang

Re: [v2 0/4] KVM: irqchip: synchronize srcu only if needed

[Christian Borntraeger]

Am 21.01.24 um 12:17 schrieb Yi Wang:
> From: Yi Wang <foxywang@tencent.com>
> 
> We found that it may cost more than 20 milliseconds very accidentally
> to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> already.
> 
> The reason is that when vmm(qemu/CloudHypervisor) invokes
> KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> might_sleep and kworker of srcu may cost some delay during this period.
> One way makes sence is setup empty irq routing when creating vm and
> so that x86/s390 don't need to setup empty/dummy irq routing.
> 
> Note: I have no s390 machine so the s390 patch has not been tested.

I just did a quick sniff and it still seems to work. No performance check etc.

Re: [v2 0/4] KVM: irqchip: synchronize srcu only if needed

[Yi Wang]

On Wed, Jan 24, 2024 at 8:14 PM Christian Borntraeger
<borntraeger@linux.ibm.com> wrote:
>
> Am 21.01.24 um 12:17 schrieb Yi Wang:
> > From: Yi Wang <foxywang@tencent.com>
> >
> > We found that it may cost more than 20 milliseconds very accidentally
> > to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> > already.
> >
> > The reason is that when vmm(qemu/CloudHypervisor) invokes
> > KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> > might_sleep and kworker of srcu may cost some delay during this period.
> > One way makes sence is setup empty irq routing when creating vm and
> > so that x86/s390 don't need to setup empty/dummy irq routing.
> >
> > Note: I have no s390 machine so the s390 patch has not been tested.
>
> I just did a quick sniff and it still seems to work. No performance check etc.

Thanks very much, Christian!

---
Best wishes
Yi Wang