LinuxLists.cc - [v3 0/3] KVM: irqchip: synchronize srcu only if needed

2024-01-24 11:35:48

Subject: [v3 0/3] KVM: irqchip: synchronize srcu only if needed

2024-01-24 11:52:43

Subject: [v3 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

From: Yi Wang <[email protected]>

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 <[email protected]>
---
arch/s390/kvm/kvm-s390.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index acc81ca6492e..dec3c026a6c1 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2999,14 +2999,7 @@ 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);
- }
+ r = 0;
break;
}
case KVM_SET_DEVICE_ATTR: {
--
2.39.3

2024-01-24 11:53:48

by Yi Wang

[permalink] [raw]

Subject: [v3 2/3] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP

From: Yi Wang <[email protected]>

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 <[email protected]>
---
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

2024-02-09 09:11:17

by Dongli Zhang

[permalink] [raw]

Subject: Re: [v3 0/3] KVM: irqchip: synchronize srcu only if needed

Hi Yi,

On 1/24/24 03:34, Yi Wang wrote:
> From: Yi Wang <[email protected]>
>
> 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.

Would you mind explaining the reason that the *number of VMs* matters, as
KVM_CAP_SPLIT_IRQCHIP is a per-VM cap?

Or it meant it is more likely to have some VM workload impacted by the
synchronize_srcu_expedited() as in prior discussion?

https://lore.kernel.org/kvm/CAN35MuSkQf0XmBZ5ZXGhcpUCGD-kKoyTv9G7ya4QVD1xiqOxLg@mail.gmail.com/

Thank you very much!

Dongli Zhang

>
> 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:
> ----------
> v3:
> - squash setup empty routing function and use of that into one commit
> - drop the comment in s390 part
>
> 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://urldefense.com/v3/__https://lore.kernel.org/kvm/[email protected]/__;!!ACWV5N9M2RV99hQ!LjwKfBaGVl3u1l9YQSskg_1RU6278h2-fYnYLsoihF9i43aq73eIDqolGzOmeRvO8UlPreQHLqXEL1bAuw$
>
> Yi Wang (3):
> 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 | 9 +--------
> 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, 25 insertions(+), 17 deletions(-)
>

2024-02-09 10:18:26

by Yi Wang

[permalink] [raw]

Subject: Re: [v3 0/3] KVM: irqchip: synchronize srcu only if needed

Hi Dongli,

Thanks for the reply and Happy Spring Festival to all :)

On Fri, Feb 9, 2024 at 5:00 PM Dongli Zhang <[email protected]> wrote:
>
> Hi Yi,
>
> On 1/24/24 03:34, Yi Wang wrote:
> > From: Yi Wang <[email protected]>
> >
> > 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.
>
> Would you mind explaining the reason that the *number of VMs* matters, as
> KVM_CAP_SPLIT_IRQCHIP is a per-VM cap?
>
> Or it meant it is more likely to have some VM workload impacted by the
> synchronize_srcu_expedited() as in prior discussion?
>
> https://lore.kernel.org/kvm/CAN35MuSkQf0XmBZ5ZXGhcpUCGD-kKoyTv9G7ya4QVD1xiqOxLg@mail.gmail.com/
>

The actual reason is might_sleep() and the kworker in
synchronize_srcu_expedited(),
which may cause some delay when there are pretty many threads in the host, so
"number of VMs" is just one of the situations which can trigger the issue :)

> Thank you very much!
>
> Dongli Zhang
>
> >
> > 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:
> > ----------
> > v3:
> > - squash setup empty routing function and use of that into one commit
> > - drop the comment in s390 part
> >
> > 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://urldefense.com/v3/__https://lore.kernel.org/kvm/[email protected]/__;!!ACWV5N9M2RV99hQ!LjwKfBaGVl3u1l9YQSskg_1RU6278h2-fYnYLsoihF9i43aq73eIDqolGzOmeRvO8UlPreQHLqXEL1bAuw$
> >
> > Yi Wang (3):
> > 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 | 9 +--------
> > 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, 25 insertions(+), 17 deletions(-)
> >

--
---
Best wishes
Yi Wang