Re: [PATCH v1 3/3] virtio-pci: defer to commit kvm irq routing when enable msi/msix

[Michael S. Tsirkin]

On Tue, Feb 28, 2023 at 05:39:37PM +0800, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> All unmasked vectors will be setup in msix_set_vector_notifiers(), which
> is a time-consuming operation because each vector need to be submit to
> KVM once. It's even worse if the VM has several devices and each devices
> has dozens of vectors.
> 
> We can defer and commit the vectors in batch, just like the commit dc580d51f7
> ("vfio: defer to commit kvm irq routing when enable msi/msix"),
> 
> The can reduce 80% of the time spending on virtio_pci_set_guest_notifiers().

cover letter also refers to 80%. what about patch 1 then? does it 
contribute some of this gain?

> Signed-off-by: Longpeng <longpeng2@huawei.com>

In the age of language models there's no longer any excuse to post
agrammatical commit messages. Please just give your text to one of these
to correct.

I prompted: "please correct grammar in the following text"
and got back:

        All unmasked vectors will be set up in
        msix_set_vector_notifiers(), which is a time-consuming operation because
        each vector needs to be submitted to KVM once. It's even worse if the VM
        has several devices and each device has dozens of vectors.

        We can defer and commit the vectors in batches, just like the
        commit dc580d51f7 ("vfio: defer to commit kvm irq routing when enabling
        msi/msix").

        This can reduce the time spent on virtio_pci_set_guest_notifiers() by 
80%.




> ---
>  hw/virtio/virtio-pci.c     | 113 ++++++++++++++++++++++++++++++++-----
>  include/hw/virtio/virtio.h |   1 +
>  2 files changed, 99 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 5fd02b7cb8..13f9c31009 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -51,15 +51,22 @@
>  
>  /* Protected by the BQL */
>  static KVMRouteChange virtio_pci_route_change;
> +static unsigned virtio_pci_route_change_depth;
>  
>  static inline void virtio_pci_begin_route_changes(void)
>  {
> -    virtio_pci_route_change = kvm_irqchip_begin_route_changes(kvm_state);
> +    if (!virtio_pci_route_change_depth) {
> +        virtio_pci_route_change = kvm_irqchip_begin_route_changes(kvm_state);
> +    }
> +    virtio_pci_route_change_depth++;
>  }
>  
>  static inline void virtio_pci_commit_route_changes(void)
>  {
> -    kvm_irqchip_commit_route_changes(&virtio_pci_route_change);
> +    virtio_pci_route_change_depth--;
> +    if (!virtio_pci_route_change_depth) {
> +        kvm_irqchip_commit_route_changes(&virtio_pci_route_change);
> +    }
>  }
>  
>  static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -976,6 +983,88 @@ static void 
> kvm_virtio_pci_vector_config_release(VirtIOPCIProxy *proxy)
>      kvm_virtio_pci_vector_release_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
>  }
>  
> +static int virtio_pci_vector_do_unmask(VirtIOPCIProxy *proxy,
> +                                       unsigned int queue_no,
> +                                       unsigned int vector,
> +                                       EventNotifier *n)
> +{
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    int ret = 0;
> +
> +    /*
> +     * If guest supports masking, irqfd is already setup, unmask it.
> +     * Otherwise, set it up now.
> +     */
> +    if (vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
> +        k->guest_notifier_mask(vdev, queue_no, false);
> +        /* Test after unmasking to avoid losing events. */
> +        if (k->guest_notifier_pending &&
> +            k->guest_notifier_pending(vdev, queue_no)) {
> +            event_notifier_set(n);
> +        }
> +    } else {
> +        ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> +    }
> +
> +    return ret;
> +}
> +
> +static void virtio_pci_prepare_kvm_msi_virq_batch(VirtIOPCIProxy *proxy)
> +{
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    assert(!vdev->defer_kvm_irq_routing);
> +    vdev->defer_kvm_irq_routing = true;
> +    virtio_pci_begin_route_changes();

move this out of here please - otherwise it's not clear each begin
is matched by commit.  in fact just open code this function.

> +}
> +
> +static void virtio_pci_commit_kvm_msi_virq_batch(VirtIOPCIProxy *proxy)
> +{
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    PCIDevice *dev = &proxy->pci_dev;
> +    VirtQueue *vq;
> +    EventNotifier *n;
> +    int vector, index;
> +    int ret;
> +
> +    assert(vdev->defer_kvm_irq_routing);
> +    virtio_pci_commit_route_changes();
> +    vdev->defer_kvm_irq_routing = false;
> +
> +    if (!msix_enabled(dev)) {
> +        return;
> +    }
> +
> +    /* Unmask all unmasked vectors */
> +    for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> +        if (msix_is_masked(dev, vector)) {
> +            continue;
> +        }
> +
> +        vq = virtio_vector_first_queue(vdev, vector);
> +        while (vq) {
> +            index = virtio_get_queue_index(vq);
> +            if (!virtio_queue_get_num(vdev, index)) {
> +                break;
> +            }
> +            if (index < proxy->nvqs_with_notifiers) {
> +                n = virtio_queue_get_guest_notifier(vq);
> +                ret = virtio_pci_vector_do_unmask(proxy, index, vector, n);
> +                assert(ret >= 0);
> +            }
> +            vq = virtio_vector_next_queue(vq);
> +        }
> +
> +        if (vector == vdev->config_vector) {
> +            n = virtio_config_get_guest_notifier(vdev);
> +            ret = virtio_pci_vector_do_unmask(proxy, VIRTIO_CONFIG_IRQ_IDX,
> +                                              vector, n);
> +            assert(ret >= 0);
> +        }
> +    }
> +}
> +
>  static int virtio_pci_one_vector_unmask(VirtIOPCIProxy *proxy,
>                                         unsigned int queue_no,
>                                         unsigned int vector,
> @@ -983,7 +1072,6 @@ static int virtio_pci_one_vector_unmask(VirtIOPCIProxy 
> *proxy,
>                                         EventNotifier *n)
>  {
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>      VirtIOIRQFD *irqfd;
>      int ret = 0;
>  
> @@ -1002,19 +1090,10 @@ static int 
> virtio_pci_one_vector_unmask(VirtIOPCIProxy *proxy,
>          }
>      }
>  
> -    /* If guest supports masking, irqfd is already setup, unmask it.
> -     * Otherwise, set it up now.
> -     */
> -    if (vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
> -        k->guest_notifier_mask(vdev, queue_no, false);
> -        /* Test after unmasking to avoid losing events. */
> -        if (k->guest_notifier_pending &&
> -            k->guest_notifier_pending(vdev, queue_no)) {
> -            event_notifier_set(n);
> -        }
> -    } else {
> -        ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> +    if (!vdev->defer_kvm_irq_routing) {
> +        ret = virtio_pci_vector_do_unmask(proxy, queue_no, vector, n);
>      }
> +
>      return ret;
>  }
>  
> @@ -1284,12 +1363,16 @@ static int virtio_pci_set_guest_notifiers(DeviceState 
> *d, int nvqs, bool assign)
>              }
>          }
>  
> +        virtio_pci_prepare_kvm_msi_virq_batch(proxy);
> +
>          r = msix_set_vector_notifiers(&proxy->pci_dev, 
> virtio_pci_vector_unmask,
>                                        virtio_pci_vector_mask,
>                                        virtio_pci_vector_poll);
>          if (r < 0) {
>              goto notifiers_error;
>          }
> +
> +        virtio_pci_commit_kvm_msi_virq_batch(proxy);
>      }
>  
>      return 0;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 77c6c55929..9d82831350 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -147,6 +147,7 @@ struct VirtIODevice
>      bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
>      bool disable_legacy_check;
>      bool vhost_started;
> +    bool defer_kvm_irq_routing;

Can't we avoid leaking kvm things all over the place?
What does this flag even mean?

>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;
> -- 
> 2.23.0