Re: [v5 1/3] KVM: setup empty irq routing when create vm

[Christian Borntraeger]

Am 06.05.24 um 12:17 schrieb Yi Wang:
> From: Yi Wang <foxywang@xxxxxxxxxxx>
>
> 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().
>
> Using the new adding function, we can 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@xxxxxxxxxxx>

We have recently looked into cpu consumption for virtio.
So interestingly enough, this increases cpu consumption for things like uperf
ping pong on s390.
Bisect points to this commit.
I originally thought that this is a no-op for s390, but it is not.

The reasons seems to be that nr_rt_entries is now 1 instead of 0 making every
interrupt inject more expensive as we no longer drop out in

int kvm_irq_map_gsi(struct kvm *kvm,
                    struct kvm_kernel_irq_routing_entry *entries, int gsi)
{
        struct kvm_irq_routing_table *irq_rt;
        struct kvm_kernel_irq_routing_entry *e;
        int n = 0;

        irq_rt = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
                                        lockdep_is_held(&kvm->irq_lock));
        if (irq_rt && gsi < irq_rt->nr_rt_entries) {  <---------


[...]

> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 1e567d1f6d3d..ec1fda7fffea 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -237,3 +237,26 @@ int kvm_set_irq_routing(struct kvm *kvm,
>   
>   	return r;
>   }
> +
> +/*
> + * Alloc empty irq routing.
> + * Called only during vm creation, because we don't synchronize_srcu here.
> + */
> +int kvm_init_irq_routing(struct kvm *kvm)
> +{
> +	struct kvm_irq_routing_table *new;
> +	int chip_size;
> +
> +	new = kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new->nr_rt_entries = 1;

Does anyone see a problem with changing that to

	new->nr_rt_entries = 0;

?