From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sridhar Samudrala Subject: Re: [PATCH] vhost: force vhost off for non-MSI guests Date: Thu, 20 Jan 2011 15:43:26 -0800 Message-ID: <1295567006.24174.244.camel@sridhar.beaverton.ibm.com> References: <20110120153521.GA24357@redhat.com> <1295541113.24174.4.camel@sridhar.beaverton.ibm.com> <20110120174721.GA25813@redhat.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Alex Williamson , Juan Quintela , jasowang@redhat.com, Jes.Sorensen@redhat.com, kvm@vger.kernel.org, qemu-devel@nongnu.org To: "Michael S. Tsirkin" Return-path: Received: from e9.ny.us.ibm.com ([32.97.182.139]:50122 "EHLO e9.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755869Ab1ATXna (ORCPT ); Thu, 20 Jan 2011 18:43:30 -0500 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e9.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p0KNJ4g8027878 for ; Thu, 20 Jan 2011 18:19:06 -0500 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 252A9728051 for ; Thu, 20 Jan 2011 18:43:29 -0500 (EST) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p0KNhSUl416130 for ; Thu, 20 Jan 2011 18:43:28 -0500 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p0KNhSs5025973 for ; Thu, 20 Jan 2011 16:43:28 -0700 In-Reply-To: <20110120174721.GA25813@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, 2011-01-20 at 19:47 +0200, Michael S. Tsirkin wrote: > On Thu, Jan 20, 2011 at 08:31:53AM -0800, Sridhar Samudrala wrote: > > On Thu, 2011-01-20 at 17:35 +0200, 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 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? > > > > Should this error be EVHOST_DISABLED rather than EVIRTIO_DISABLED? > > > > -Sridhar > > The error is reported by virtio-pci which does not know about vhost. > I started with EVIRTIO_MSIX_DISABLED and made is shorter. > Would EVIRTIO_MSIX_DISABLED be better? I think so. This makes it more clear. -Sridhar > > > > > > > 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=60030 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pg4AA-00037n-Of for qemu-devel@nongnu.org; Thu, 20 Jan 2011 18:43:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pg4A9-0000Hr-Dx for qemu-devel@nongnu.org; Thu, 20 Jan 2011 18:43:34 -0500 Received: from e38.co.us.ibm.com ([32.97.110.159]:46744) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pg4A9-0000GZ-7b for qemu-devel@nongnu.org; Thu, 20 Jan 2011 18:43:33 -0500 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by e38.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p0KNTLSX019570 for ; Thu, 20 Jan 2011 16:29:21 -0700 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p0KNhSAt143430 for ; Thu, 20 Jan 2011 16:43:28 -0700 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p0KNhSs1025973 for ; Thu, 20 Jan 2011 16:43:28 -0700 From: Sridhar Samudrala In-Reply-To: <20110120174721.GA25813@redhat.com> References: <20110120153521.GA24357@redhat.com> <1295541113.24174.4.camel@sridhar.beaverton.ibm.com> <20110120174721.GA25813@redhat.com> Content-Type: text/plain Date: Thu, 20 Jan 2011 15:43:26 -0800 Message-Id: <1295567006.24174.244.camel@sridhar.beaverton.ibm.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] vhost: force vhost off for non-MSI guests 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 Thu, 2011-01-20 at 19:47 +0200, Michael S. Tsirkin wrote: > On Thu, Jan 20, 2011 at 08:31:53AM -0800, Sridhar Samudrala wrote: > > On Thu, 2011-01-20 at 17:35 +0200, 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 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? > > > > Should this error be EVHOST_DISABLED rather than EVIRTIO_DISABLED? > > > > -Sridhar > > The error is reported by virtio-pci which does not know about vhost. > I started with EVIRTIO_MSIX_DISABLED and made is shorter. > Would EVIRTIO_MSIX_DISABLED be better? I think so. This makes it more clear. -Sridhar > > > > > > > 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