From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [Qemu-devel] [PATCH] vhost: force vhost off for non-MSI guests Date: Thu, 20 Jan 2011 09:43:57 -0600 Message-ID: <4D38583D.9010602@codemonkey.ws> References: <20110120153521.GA24357@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Alex Williamson , Juan Quintela , jasowang@redhat.com, Jes.Sorensen@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from mail-yi0-f46.google.com ([209.85.218.46]:57254 "EHLO mail-yi0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755907Ab1ATPn7 (ORCPT ); Thu, 20 Jan 2011 10:43:59 -0500 Received: by yib18 with SMTP id 18so206919yib.19 for ; Thu, 20 Jan 2011 07:43:59 -0800 (PST) In-Reply-To: <20110120153521.GA24357@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 01/20/2011 09:35 AM, Michael S. Tsirkin wrote: > When MSI is off, each interrupt needs to be bounced through the io > thread when it's set/cleared, so vhost-net causes more context switches and > higher CPU utilization than userspace virtio which handles networking in > the same thread. > > We'll need to fix this by adding level irq support in kvm irqfd, > for now disable vhost-net in these configurations. > > Signed-off-by: Michael S. Tsirkin > I actually think this should be a terminal error. The user asks for vhost-net, if we cannot enable it, we should exit. Or we should warn the user that they should expect bad performance. Silently doing something that the user has explicitly asked us not to do is not a good behavior. Regards, Anthony Liguori > --- > > I need to report some error from virtio-pci > that would be handled specially (disable but don't > report an error) so I wanted one that's never likely to be used by a > userspace ioctl. I selected ERANGE but it'd > be easy to switch to something else. Comments? > > hw/vhost.c | 4 +++- > hw/virtio-net.c | 6 ++++-- > hw/virtio-pci.c | 3 +++ > hw/virtio.h | 2 ++ > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/hw/vhost.c b/hw/vhost.c > index 1d09ed0..c79765a 100644 > --- a/hw/vhost.c > +++ b/hw/vhost.c > @@ -649,7 +649,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > > r = vdev->binding->set_guest_notifiers(vdev->binding_opaque, true); > if (r< 0) { > - fprintf(stderr, "Error binding guest notifier: %d\n", -r); > + if (r != -EVIRTIO_DISABLED) { > + fprintf(stderr, "Error binding guest notifier: %d\n", -r); > + } > goto fail_notifiers; > } > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index ccb3e63..5de3fee 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -121,8 +121,10 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > if (!n->vhost_started) { > int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer),&n->vdev); > if (r< 0) { > - error_report("unable to start vhost net: %d: " > - "falling back on userspace virtio", -r); > + if (r != -EVIRTIO_DISABLED) { > + error_report("unable to start vhost net: %d: " > + "falling back on userspace virtio", -r); > + } > } else { > n->vhost_started = 1; > } > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index dd8887a..dbf4be0 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -628,6 +628,9 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); > > if (assign) { > + if (!msix_enabled(&proxy->pci_dev)) { > + return -EVIRTIO_DISABLED; > + } > int r = event_notifier_init(notifier, 0); > if (r< 0) { > return r; > diff --git a/hw/virtio.h b/hw/virtio.h > index d8546d5..53bbdba 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -98,6 +98,8 @@ typedef struct { > void (*vmstate_change)(void * opaque, bool running); > } VirtIOBindings; > > +#define EVIRTIO_DISABLED ERANGE > + > #define VIRTIO_PCI_QUEUE_MAX 64 > > #define VIRTIO_NO_VECTOR 0xffff > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33357 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pfwg5-0004BL-Er for qemu-devel@nongnu.org; Thu, 20 Jan 2011 10:44:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pfwg4-0000zd-Bp for qemu-devel@nongnu.org; Thu, 20 Jan 2011 10:44:01 -0500 Received: from mail-gy0-f173.google.com ([209.85.160.173]:37312) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pfwg4-0000zY-9G for qemu-devel@nongnu.org; Thu, 20 Jan 2011 10:44:00 -0500 Received: by gye5 with SMTP id 5so230299gye.4 for ; Thu, 20 Jan 2011 07:43:59 -0800 (PST) Message-ID: <4D38583D.9010602@codemonkey.ws> Date: Thu, 20 Jan 2011 09:43:57 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] vhost: force vhost off for non-MSI guests References: <20110120153521.GA24357@redhat.com> In-Reply-To: <20110120153521.GA24357@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: kvm@vger.kernel.org, Juan Quintela , Jes.Sorensen@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, Alex Williamson On 01/20/2011 09:35 AM, Michael S. Tsirkin wrote: > When MSI is off, each interrupt needs to be bounced through the io > thread when it's set/cleared, so vhost-net causes more context switches and > higher CPU utilization than userspace virtio which handles networking in > the same thread. > > We'll need to fix this by adding level irq support in kvm irqfd, > for now disable vhost-net in these configurations. > > Signed-off-by: Michael S. Tsirkin > I actually think this should be a terminal error. The user asks for vhost-net, if we cannot enable it, we should exit. Or we should warn the user that they should expect bad performance. Silently doing something that the user has explicitly asked us not to do is not a good behavior. Regards, Anthony Liguori > --- > > I need to report some error from virtio-pci > that would be handled specially (disable but don't > report an error) so I wanted one that's never likely to be used by a > userspace ioctl. I selected ERANGE but it'd > be easy to switch to something else. Comments? > > hw/vhost.c | 4 +++- > hw/virtio-net.c | 6 ++++-- > hw/virtio-pci.c | 3 +++ > hw/virtio.h | 2 ++ > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/hw/vhost.c b/hw/vhost.c > index 1d09ed0..c79765a 100644 > --- a/hw/vhost.c > +++ b/hw/vhost.c > @@ -649,7 +649,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > > r = vdev->binding->set_guest_notifiers(vdev->binding_opaque, true); > if (r< 0) { > - fprintf(stderr, "Error binding guest notifier: %d\n", -r); > + if (r != -EVIRTIO_DISABLED) { > + fprintf(stderr, "Error binding guest notifier: %d\n", -r); > + } > goto fail_notifiers; > } > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index ccb3e63..5de3fee 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -121,8 +121,10 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > if (!n->vhost_started) { > int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer),&n->vdev); > if (r< 0) { > - error_report("unable to start vhost net: %d: " > - "falling back on userspace virtio", -r); > + if (r != -EVIRTIO_DISABLED) { > + error_report("unable to start vhost net: %d: " > + "falling back on userspace virtio", -r); > + } > } else { > n->vhost_started = 1; > } > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index dd8887a..dbf4be0 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -628,6 +628,9 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); > > if (assign) { > + if (!msix_enabled(&proxy->pci_dev)) { > + return -EVIRTIO_DISABLED; > + } > int r = event_notifier_init(notifier, 0); > if (r< 0) { > return r; > diff --git a/hw/virtio.h b/hw/virtio.h > index d8546d5..53bbdba 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -98,6 +98,8 @@ typedef struct { > void (*vmstate_change)(void * opaque, bool running); > } VirtIOBindings; > > +#define EVIRTIO_DISABLED ERANGE > + > #define VIRTIO_PCI_QUEUE_MAX 64 > > #define VIRTIO_NO_VECTOR 0xffff >