Re: [Qemu-devel] [PATCH] [KVM] Needless to update msi route when only msi-x entry "control" section changed

[Michael S. Tsirkin]

On Mon, May 06, 2013 at 02:52:37AM +0000, Zhanghaoyu (A) wrote:
> >> With regard to old version linux guest(e.g., rhel-5.5), in ISR processing, 
> >> mask and unmask msi-x vector every time, which result in VMEXIT, then QEMU 
> >> will invoke kvm_irqchip_update_msi_route() to ask KVM hypervisor to update 
> >> the VM irq routing table. In KVM hypervisor, synchronizing RCU needed 
> >> after updating routing table, so much time consumed for waiting in 
> >> wait_rcu_gp(). So CPU usage in VM is so high, while from the view of host, 
> >> VM's total CPU usage is so low. 
> >> Masking/unmasking msi-x vector only set msi-x entry "control" section, 
> >> needless to update VM irq routing table.
> >> 
> >> Signed-off-by: Zhang Haoyu <address@hidden>
> >> Signed-off-by: Huang Weidong <address@hidden>
> >> Signed-off-by: Qin Chuanyu <address@hidden>
> >> ---
> >> hw/i386/kvm/pci-assign.c | 3 +++
> >> 1 files changed, 3 insertions(+)
> >> 
> >> --- a/hw/i386/kvm/pci-assign.c  2013-05-04 15:53:18.000000000 +0800
> >> +++ b/hw/i386/kvm/pci-assign.c  2013-05-04 15:50:46.000000000 +0800
> >> @@ -1576,6 +1576,8 @@ static void assigned_dev_msix_mmio_write
> >>                  MSIMessage msg;
> >>                  int ret;
> >> 
> >> +                /* Needless to update msi route when only msi-x entry 
> >> "control" section changed */
> >> +                if ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) != 
> >> + PCI_MSIX_ENTRY_VECTOR_CTRL){
> >>                  msg.address = entry->addr_lo |
> >>                      ((uint64_t)entry->addr_hi << 32);
> >>                  msg.data = entry->data; @@ -1585,6 +1587,7 @@ static 
> >> void assigned_dev_msix_mmio_write
> >>                  if (ret) {
> >>                      error_report("Error updating irq routing entry (%d)", 
> >> ret);
> >>                  }
> >> +                }
> >>              }
> >>          }
> >>      }
> >> 
> >> Thanks,
> >> Zhang Haoyu
> >
> >
> >If guest wants to update the vector, it does it like this:
> >mask
> >update
> >unmask
> >and it looks like the only point where we update the vector is on unmask, so 
> >this patch will mean we don't update the vector ever.
> >
> >I'm not sure this combination (old guest + legacy device assignment
> >framework) is worth optimizing. Can you try VFIO instead?
> >
> >But if it is, the right way to do this is probably along the lines of the 
> >below patch. Want to try it out?
> >
> >diff --git a/kvm-all.c b/kvm-all.c
> >index 2d92721..afe2327 100644
> >--- a/kvm-all.c
> >+++ b/kvm-all.c
> >@@ -1006,6 +1006,11 @@ static int kvm_update_routing_entry(KVMState *s,
> >             continue;
> >         }
> > 
> >+        if (entry->type == new_entry->type &&
> >+            entry->flags == new_entry->flags &&
> >+            entry->u == new_entry->u) {
> >+            return 0;
> >+        }
> >         entry->type = new_entry->type;
> >         entry->flags = new_entry->flags;
> >         entry->u = new_entry->u;
> >
> 
> union type cannot be directly compared, I tried out below patch instead,
> --- a/kvm-all.c 2013-05-06 09:56:38.000000000 +0800
> +++ b/kvm-all.c 2013-05-06 09:56:45.000000000 +0800
> @@ -1008,6 +1008,12 @@ static int kvm_update_routing_entry(KVMS
>              continue;
>          }
> 
> +        if (entry->type == new_entry->type &&
> +            entry->flags == new_entry->flags &&
> +            !memcmp(&entry->u, &new_entry->u, sizeof(entry->u))) {
> +            return 0;
> +        }
> +
>          entry->type = new_entry->type;
>          entry->flags = new_entry->flags;
>          entry->u = new_entry->u;
> 
> MST's patch is more universal than my first patch fixed in 
> assigned_dev_msix_mmio_write().
> On the case that the msix entry's other section but "control" section is set 
> to the identical value with old entry's, MST's patch also works.
> MST's patch also works on the non-passthrough scenario.

Any numbers for either case?

> And, after MST's patch applied, the below check in 
> virtio_pci_vq_vector_unmask() can be removed.
> --- a/hw/virtio/virtio-pci.c    2013-05-04 15:53:20.000000000 +0800
> +++ b/hw/virtio/virtio-pci.c    2013-05-06 10:25:58.000000000 +0800
> @@ -619,12 +619,10 @@ static int virtio_pci_vq_vector_unmask(V
> 
>      if (proxy->vector_irqfd) {
>          irqfd = &proxy->vector_irqfd[vector];
> -        if (irqfd->msg.data != msg.data || irqfd->msg.address != 
> msg.address) {
>              ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, msg);
>              if (ret < 0) {
>                  return ret;
>              }
> -        }
>      }
> 
>      /* If guest supports masking, irqfd is already setup, unmask it.
> 
> Thanks,
> Zhang Haoyu