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

[Zhanghaoyu (A)]

>> 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.

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