From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbUlo-0002Q2-OH for qemu-devel@nongnu.org; Sun, 12 May 2013 07:48:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UbUlk-00040o-QR for qemu-devel@nongnu.org; Sun, 12 May 2013 07:48:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18094) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbUlk-00040j-Is for qemu-devel@nongnu.org; Sun, 12 May 2013 07:48:48 -0400 Date: Sun, 12 May 2013 14:48:43 +0300 From: "Michael S. Tsirkin" Message-ID: <20130512114843.GG10144@redhat.com> References: <20130505211307.GB7861@redhat.com> <20130506100009.GA16708@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] [KVM] Needless to update msi route when only msi-x entry "control" section changed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Zhanghaoyu (A)" Cc: luonengjun 00137267 , Marcelo Tosatti , qemu-devel , 00180273 , "q00170852@notesmail.huawei.com.cn" , huangweidong 00164878 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 > >> >> Signed-off-by: Huang Weidong > >> >> Signed-off-by: Qin Chuanyu > >> >> --- > >> >> 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