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

[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 <haoyu.zhang@huawei.com>
Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
---
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

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

[Michael S. Tsirkin]

On Sat, May 04, 2013 at 09:12:30AM +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 <haoyu.zhang@huawei.com>
> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
> ---
> 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;

-- 
MST

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 <haoyu.zhang@huawei.com>
>> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
>> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
>> ---
>> 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

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 <haoyu.zhang@huawei.com>
> >> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
> >> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
> >> ---
> >> 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

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 <haoyu.zhang@huawei.com>
>> >> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
>> >> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
>> >> ---
>> >> 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?
>
I'm not sure what you said exactly means. 
Do you want me to make a further statement for comparison between above two patches?
If yes, no other comments.

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

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

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 01:53:03AM +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 <haoyu.zhang@huawei.com>
> >> >> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
> >> >> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
> >> >> ---
> >> >> 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?
> >
> I'm not sure what you said exactly means. 
> Do you want me to make a further statement for comparison between above two patches?
> If yes, no other comments.


Sorry.
What I mean is that we'll need to know what is the
performance impact of my patch before we can
device whether to apply it.
Could you send this information please?

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

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 01:00:09PM +0300, Michael S. Tsirkin wrote:
> 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 <haoyu.zhang@huawei.com>
> > >> Signed-off-by: Huang Weidong <weidong.huang@huawei.com>
> > >> Signed-off-by: Qin Chuanyu <qinchuanyu@huawei.com>
> > >> ---
> > >> 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?

Ping.
Is this patch helpful to you?
If yes what is the performance gain from it?


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