qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
@ 2015-03-09  6:00 Pankaj Gupta
  2015-05-27  6:26 ` Pankaj Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Pankaj Gupta @ 2015-03-09  6:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Pankaj Gupta, jasowang, aliguori, stefanha, mst

 vhostforce was added to enable vhost when
 guest don't have MSI-X support.
 Now, we have scenarios like DPDK in Guest which dont use 
 interrupts and still use vhost. Also, performance of guests 
 without MSI-X support is getting less popular.

 Its OK to remove this extra option and enable vhost
 on the basis of vhost=ON/OFF.
 Done basic testing with vhost on/off for latest guests
 and old guests(non-msix).

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/net/vhost_net.c        | 2 +-
 hw/scsi/vhost-scsi.c      | 2 +-
 hw/virtio/vhost.c         | 6 ++----
 include/hw/virtio/vhost.h | 3 +--
 include/net/vhost_net.h   | 1 -
 net/tap.c                 | 4 +---
 net/vhost-user.c          | 1 -
 7 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 4e3a061..7139b9f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
     net->dev.vqs = net->vqs;
 
     r = vhost_dev_init(&net->dev, options->opaque,
-                       options->backend_type, options->force);
+                       options->backend_type);
     if (r < 0) {
         goto fail;
     }
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 618b0af..b15390e 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     s->dev.backend_features = 0;
 
     ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
-                         VHOST_BACKEND_TYPE_KERNEL, true);
+                         VHOST_BACKEND_TYPE_KERNEL);
     if (ret < 0) {
         error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
                    strerror(-ret));
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5a12861..ce33e04 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
 }
 
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
-                   VhostBackendType backend_type, bool force)
+                   VhostBackendType backend_type)
 {
     uint64_t features;
     int i, r;
@@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->started = false;
     hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
-    hdev->force = force;
     return 0;
 fail_vq:
     while (--i >= 0) {
@@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
 
     return !k->query_guest_notifiers ||
-           k->query_guest_notifiers(qbus->parent) ||
-           hdev->force;
+           k->query_guest_notifiers(qbus->parent);
 }
 
 /* Stop processing guest IO notifications in qemu.
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index d5593d1..27eae3e 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -46,7 +46,6 @@ struct vhost_dev {
     vhost_log_chunk_t *log;
     unsigned long long log_size;
     Error *migration_blocker;
-    bool force;
     bool memory_changed;
     hwaddr mem_changed_start_addr;
     hwaddr mem_changed_end_addr;
@@ -55,7 +54,7 @@ struct vhost_dev {
 };
 
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
-                   VhostBackendType backend_type, bool force);
+                   VhostBackendType backend_type);
 void vhost_dev_cleanup(struct vhost_dev *hdev);
 bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
 int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index b1c18a3..966a945 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
     VhostBackendType backend_type;
     NetClientState *net_backend;
     void *opaque;
-    bool force;
 } VhostNetOptions;
 
 struct vhost_net *vhost_net_init(VhostNetOptions *options);
diff --git a/net/tap.c b/net/tap.c
index 968df46..b9002f7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         }
     }
 
-    if (tap->has_vhost ? tap->vhost :
-        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
+    if (tap->has_vhost ? tap->vhost : vhostfdname) {
         VhostNetOptions options;
 
         options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
         options.net_backend = &s->nc;
-        options.force = tap->has_vhostforce && tap->vhostforce;
 
         if (tap->has_vhostfd || tap->has_vhostfds) {
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 24e050c..9966de5 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
     options.backend_type = VHOST_BACKEND_TYPE_USER;
     options.net_backend = &s->nc;
     options.opaque = s->chr;
-    options.force = s->vhostforce;
 
     s->vhost_net = vhost_net_init(&options);
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-03-09  6:00 [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter Pankaj Gupta
@ 2015-05-27  6:26 ` Pankaj Gupta
  2015-05-27  8:45   ` Jason Wang
  2015-05-27 12:00   ` Michael S. Tsirkin
  0 siblings, 2 replies; 10+ messages in thread
From: Pankaj Gupta @ 2015-05-27  6:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: jasowang, stefanha, aliguori, mst


Ping.

Can I get any suggestions on this patch.

Best regards,
Pankaj

> 
> vhostforce was added to enable vhost when
>  guest don't have MSI-X support.
>  Now, we have scenarios like DPDK in Guest which dont use
>  interrupts and still use vhost. Also, performance of guests
>  without MSI-X support is getting less popular.
> 
>  Its OK to remove this extra option and enable vhost
>  on the basis of vhost=ON/OFF.
>  Done basic testing with vhost on/off for latest guests
>  and old guests(non-msix).
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/net/vhost_net.c        | 2 +-
>  hw/scsi/vhost-scsi.c      | 2 +-
>  hw/virtio/vhost.c         | 6 ++----
>  include/hw/virtio/vhost.h | 3 +--
>  include/net/vhost_net.h   | 1 -
>  net/tap.c                 | 4 +---
>  net/vhost-user.c          | 1 -
>  7 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 4e3a061..7139b9f 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> *options)
>      net->dev.vqs = net->vqs;
>  
>      r = vhost_dev_init(&net->dev, options->opaque,
> -                       options->backend_type, options->force);
> +                       options->backend_type);
>      if (r < 0) {
>          goto fail;
>      }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 618b0af..b15390e 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
> **errp)
>      s->dev.backend_features = 0;
>  
>      ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> -                         VHOST_BACKEND_TYPE_KERNEL, true);
> +                         VHOST_BACKEND_TYPE_KERNEL);
>      if (ret < 0) {
>          error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
>                     strerror(-ret));
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5a12861..ce33e04 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct
> vhost_virtqueue *vq)
>  }
>  
>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> -                   VhostBackendType backend_type, bool force)
> +                   VhostBackendType backend_type)
>  {
>      uint64_t features;
>      int i, r;
> @@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>      hdev->started = false;
>      hdev->memory_changed = false;
>      memory_listener_register(&hdev->memory_listener, &address_space_memory);
> -    hdev->force = force;
>      return 0;
>  fail_vq:
>      while (--i >= 0) {
> @@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice
> *vdev)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>  
>      return !k->query_guest_notifiers ||
> -           k->query_guest_notifiers(qbus->parent) ||
> -           hdev->force;
> +           k->query_guest_notifiers(qbus->parent);
>  }
>  
>  /* Stop processing guest IO notifications in qemu.
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index d5593d1..27eae3e 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -46,7 +46,6 @@ struct vhost_dev {
>      vhost_log_chunk_t *log;
>      unsigned long long log_size;
>      Error *migration_blocker;
> -    bool force;
>      bool memory_changed;
>      hwaddr mem_changed_start_addr;
>      hwaddr mem_changed_end_addr;
> @@ -55,7 +54,7 @@ struct vhost_dev {
>  };
>  
>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> -                   VhostBackendType backend_type, bool force);
> +                   VhostBackendType backend_type);
>  void vhost_dev_cleanup(struct vhost_dev *hdev);
>  bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
>  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index b1c18a3..966a945 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
>      VhostBackendType backend_type;
>      NetClientState *net_backend;
>      void *opaque;
> -    bool force;
>  } VhostNetOptions;
>  
>  struct vhost_net *vhost_net_init(VhostNetOptions *options);
> diff --git a/net/tap.c b/net/tap.c
> index 968df46..b9002f7 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
>          }
>      }
>  
> -    if (tap->has_vhost ? tap->vhost :
> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> +    if (tap->has_vhost ? tap->vhost : vhostfdname) {
>          VhostNetOptions options;
>  
>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
>          options.net_backend = &s->nc;
> -        options.force = tap->has_vhostforce && tap->vhostforce;
>  
>          if (tap->has_vhostfd || tap->has_vhostfds) {
>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 24e050c..9966de5 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
>      options.backend_type = VHOST_BACKEND_TYPE_USER;
>      options.net_backend = &s->nc;
>      options.opaque = s->chr;
> -    options.force = s->vhostforce;
>  
>      s->vhost_net = vhost_net_init(&options);
>  
> --
> 1.8.3.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-27  6:26 ` Pankaj Gupta
@ 2015-05-27  8:45   ` Jason Wang
  2015-05-27 11:57     ` Michael S. Tsirkin
  2015-05-27 12:00   ` Michael S. Tsirkin
  1 sibling, 1 reply; 10+ messages in thread
From: Jason Wang @ 2015-05-27  8:45 UTC (permalink / raw)
  To: Pankaj Gupta, qemu-devel; +Cc: aliguori, stefanha, mst



On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
> Ping.
>
> Can I get any suggestions on this patch.
>
> Best regards,
> Pankaj
>
>> vhostforce was added to enable vhost when
>>  guest don't have MSI-X support.
>>  Now, we have scenarios like DPDK in Guest which dont use
>>  interrupts and still use vhost. Also, performance of guests
>>  without MSI-X support is getting less popular.
>>
>>  Its OK to remove this extra option and enable vhost
>>  on the basis of vhost=ON/OFF.
>>  Done basic testing with vhost on/off for latest guests
>>  and old guests(non-msix).
>>
>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>

Looks good. Two questions:

- Did libvirt use this? if not, we may want to drop vhostfore option
completely.
- If vhostforce=off still attractable (I guess not)? If yes, we may want
to simply make vhostforce default to true?

Thanks
>> ---
>>  hw/net/vhost_net.c        | 2 +-
>>  hw/scsi/vhost-scsi.c      | 2 +-
>>  hw/virtio/vhost.c         | 6 ++----
>>  include/hw/virtio/vhost.h | 3 +--
>>  include/net/vhost_net.h   | 1 -
>>  net/tap.c                 | 4 +---
>>  net/vhost-user.c          | 1 -
>>  7 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 4e3a061..7139b9f 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
>> *options)
>>      net->dev.vqs = net->vqs;
>>  
>>      r = vhost_dev_init(&net->dev, options->opaque,
>> -                       options->backend_type, options->force);
>> +                       options->backend_type);
>>      if (r < 0) {
>>          goto fail;
>>      }
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 618b0af..b15390e 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
>> **errp)
>>      s->dev.backend_features = 0;
>>  
>>      ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
>> -                         VHOST_BACKEND_TYPE_KERNEL, true);
>> +                         VHOST_BACKEND_TYPE_KERNEL);
>>      if (ret < 0) {
>>          error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
>>                     strerror(-ret));
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 5a12861..ce33e04 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct
>> vhost_virtqueue *vq)
>>  }
>>  
>>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> -                   VhostBackendType backend_type, bool force)
>> +                   VhostBackendType backend_type)
>>  {
>>      uint64_t features;
>>      int i, r;
>> @@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>>      hdev->started = false;
>>      hdev->memory_changed = false;
>>      memory_listener_register(&hdev->memory_listener, &address_space_memory);
>> -    hdev->force = force;
>>      return 0;
>>  fail_vq:
>>      while (--i >= 0) {
>> @@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice
>> *vdev)
>>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>  
>>      return !k->query_guest_notifiers ||
>> -           k->query_guest_notifiers(qbus->parent) ||
>> -           hdev->force;
>> +           k->query_guest_notifiers(qbus->parent);
>>  }
>>  
>>  /* Stop processing guest IO notifications in qemu.
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index d5593d1..27eae3e 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -46,7 +46,6 @@ struct vhost_dev {
>>      vhost_log_chunk_t *log;
>>      unsigned long long log_size;
>>      Error *migration_blocker;
>> -    bool force;
>>      bool memory_changed;
>>      hwaddr mem_changed_start_addr;
>>      hwaddr mem_changed_end_addr;
>> @@ -55,7 +54,7 @@ struct vhost_dev {
>>  };
>>  
>>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> -                   VhostBackendType backend_type, bool force);
>> +                   VhostBackendType backend_type);
>>  void vhost_dev_cleanup(struct vhost_dev *hdev);
>>  bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
>>  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>> index b1c18a3..966a945 100644
>> --- a/include/net/vhost_net.h
>> +++ b/include/net/vhost_net.h
>> @@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
>>      VhostBackendType backend_type;
>>      NetClientState *net_backend;
>>      void *opaque;
>> -    bool force;
>>  } VhostNetOptions;
>>  
>>  struct vhost_net *vhost_net_init(VhostNetOptions *options);
>> diff --git a/net/tap.c b/net/tap.c
>> index 968df46..b9002f7 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions
>> *tap, NetClientState *peer,
>>          }
>>      }
>>  
>> -    if (tap->has_vhost ? tap->vhost :
>> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
>> +    if (tap->has_vhost ? tap->vhost : vhostfdname) {
>>          VhostNetOptions options;
>>  
>>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
>>          options.net_backend = &s->nc;
>> -        options.force = tap->has_vhostforce && tap->vhostforce;
>>  
>>          if (tap->has_vhostfd || tap->has_vhostfds) {
>>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 24e050c..9966de5 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
>>      options.backend_type = VHOST_BACKEND_TYPE_USER;
>>      options.net_backend = &s->nc;
>>      options.opaque = s->chr;
>> -    options.force = s->vhostforce;
>>  
>>      s->vhost_net = vhost_net_init(&options);
>>  
>> --
>> 1.8.3.1
>>
>>
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-27  8:45   ` Jason Wang
@ 2015-05-27 11:57     ` Michael S. Tsirkin
  2015-05-28  3:21       ` Jason Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2015-05-27 11:57 UTC (permalink / raw)
  To: Jason Wang; +Cc: Pankaj Gupta, qemu-devel, stefanha, aliguori

On Wed, May 27, 2015 at 04:45:34PM +0800, Jason Wang wrote:
> 
> 
> On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
> > Ping.
> >
> > Can I get any suggestions on this patch.
> >
> > Best regards,
> > Pankaj
> >
> >> vhostforce was added to enable vhost when
> >>  guest don't have MSI-X support.
> >>  Now, we have scenarios like DPDK in Guest which dont use
> >>  interrupts and still use vhost. Also, performance of guests
> >>  without MSI-X support is getting less popular.
> >>
> >>  Its OK to remove this extra option and enable vhost
> >>  on the basis of vhost=ON/OFF.
> >>  Done basic testing with vhost on/off for latest guests
> >>  and old guests(non-msix).
> >>
> >> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> 
> Looks good. Two questions:
> 
> - Did libvirt use this? if not, we may want to drop vhostfore option
> completely.

Yes, it did.

> - If vhostforce=off still attractable (I guess not)? If yes, we may want
> to simply make vhostforce default to true?
> 
> Thanks
> >> ---
> >>  hw/net/vhost_net.c        | 2 +-
> >>  hw/scsi/vhost-scsi.c      | 2 +-
> >>  hw/virtio/vhost.c         | 6 ++----
> >>  include/hw/virtio/vhost.h | 3 +--
> >>  include/net/vhost_net.h   | 1 -
> >>  net/tap.c                 | 4 +---
> >>  net/vhost-user.c          | 1 -
> >>  7 files changed, 6 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index 4e3a061..7139b9f 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> >> *options)
> >>      net->dev.vqs = net->vqs;
> >>  
> >>      r = vhost_dev_init(&net->dev, options->opaque,
> >> -                       options->backend_type, options->force);
> >> +                       options->backend_type);
> >>      if (r < 0) {
> >>          goto fail;
> >>      }
> >> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> >> index 618b0af..b15390e 100644
> >> --- a/hw/scsi/vhost-scsi.c
> >> +++ b/hw/scsi/vhost-scsi.c
> >> @@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
> >> **errp)
> >>      s->dev.backend_features = 0;
> >>  
> >>      ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> >> -                         VHOST_BACKEND_TYPE_KERNEL, true);
> >> +                         VHOST_BACKEND_TYPE_KERNEL);
> >>      if (ret < 0) {
> >>          error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> >>                     strerror(-ret));
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 5a12861..ce33e04 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct
> >> vhost_virtqueue *vq)
> >>  }
> >>  
> >>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> -                   VhostBackendType backend_type, bool force)
> >> +                   VhostBackendType backend_type)
> >>  {
> >>      uint64_t features;
> >>      int i, r;
> >> @@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >>      hdev->started = false;
> >>      hdev->memory_changed = false;
> >>      memory_listener_register(&hdev->memory_listener, &address_space_memory);
> >> -    hdev->force = force;
> >>      return 0;
> >>  fail_vq:
> >>      while (--i >= 0) {
> >> @@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice
> >> *vdev)
> >>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >>  
> >>      return !k->query_guest_notifiers ||
> >> -           k->query_guest_notifiers(qbus->parent) ||
> >> -           hdev->force;
> >> +           k->query_guest_notifiers(qbus->parent);
> >>  }
> >>  
> >>  /* Stop processing guest IO notifications in qemu.
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index d5593d1..27eae3e 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -46,7 +46,6 @@ struct vhost_dev {
> >>      vhost_log_chunk_t *log;
> >>      unsigned long long log_size;
> >>      Error *migration_blocker;
> >> -    bool force;
> >>      bool memory_changed;
> >>      hwaddr mem_changed_start_addr;
> >>      hwaddr mem_changed_end_addr;
> >> @@ -55,7 +54,7 @@ struct vhost_dev {
> >>  };
> >>  
> >>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> -                   VhostBackendType backend_type, bool force);
> >> +                   VhostBackendType backend_type);
> >>  void vhost_dev_cleanup(struct vhost_dev *hdev);
> >>  bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
> >>  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> >> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> >> index b1c18a3..966a945 100644
> >> --- a/include/net/vhost_net.h
> >> +++ b/include/net/vhost_net.h
> >> @@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
> >>      VhostBackendType backend_type;
> >>      NetClientState *net_backend;
> >>      void *opaque;
> >> -    bool force;
> >>  } VhostNetOptions;
> >>  
> >>  struct vhost_net *vhost_net_init(VhostNetOptions *options);
> >> diff --git a/net/tap.c b/net/tap.c
> >> index 968df46..b9002f7 100644
> >> --- a/net/tap.c
> >> +++ b/net/tap.c
> >> @@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions
> >> *tap, NetClientState *peer,
> >>          }
> >>      }
> >>  
> >> -    if (tap->has_vhost ? tap->vhost :
> >> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> >> +    if (tap->has_vhost ? tap->vhost : vhostfdname) {
> >>          VhostNetOptions options;
> >>  
> >>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
> >>          options.net_backend = &s->nc;
> >> -        options.force = tap->has_vhostforce && tap->vhostforce;
> >>  
> >>          if (tap->has_vhostfd || tap->has_vhostfds) {
> >>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> >> diff --git a/net/vhost-user.c b/net/vhost-user.c
> >> index 24e050c..9966de5 100644
> >> --- a/net/vhost-user.c
> >> +++ b/net/vhost-user.c
> >> @@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
> >>      options.backend_type = VHOST_BACKEND_TYPE_USER;
> >>      options.net_backend = &s->nc;
> >>      options.opaque = s->chr;
> >> -    options.force = s->vhostforce;
> >>  
> >>      s->vhost_net = vhost_net_init(&options);
> >>  
> >> --
> >> 1.8.3.1
> >>
> >>
> >>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-27  6:26 ` Pankaj Gupta
  2015-05-27  8:45   ` Jason Wang
@ 2015-05-27 12:00   ` Michael S. Tsirkin
  2015-05-29  4:59     ` Pankaj Gupta
  1 sibling, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2015-05-27 12:00 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: jasowang, stefanha, qemu-devel, aliguori

On Wed, May 27, 2015 at 02:26:43AM -0400, Pankaj Gupta wrote:
> 
> Ping.
> 
> Can I get any suggestions on this patch.
> 
> Best regards,
> Pankaj



> > 
> > vhostforce was added to enable vhost when
> >  guest don't have MSI-X support.
> >  Now, we have scenarios like DPDK in Guest which dont use
> >  interrupts and still use vhost. Also, performance of guests
> >  without MSI-X support is getting less popular.
> > 
> >  Its OK to remove this extra option and enable vhost
> >  on the basis of vhost=ON/OFF.
> >  Done basic testing with vhost on/off for latest guests
> >  and old guests(non-msix).
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>

I think this silently changes the command line semantics:
previously vhostforce=on implies vhost=on, with this patch
it does not.

> > ---
> >  hw/net/vhost_net.c        | 2 +-
> >  hw/scsi/vhost-scsi.c      | 2 +-
> >  hw/virtio/vhost.c         | 6 ++----
> >  include/hw/virtio/vhost.h | 3 +--
> >  include/net/vhost_net.h   | 1 -
> >  net/tap.c                 | 4 +---
> >  net/vhost-user.c          | 1 -
> >  7 files changed, 6 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 4e3a061..7139b9f 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > *options)
> >      net->dev.vqs = net->vqs;
> >  
> >      r = vhost_dev_init(&net->dev, options->opaque,
> > -                       options->backend_type, options->force);
> > +                       options->backend_type);
> >      if (r < 0) {
> >          goto fail;
> >      }
> > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > index 618b0af..b15390e 100644
> > --- a/hw/scsi/vhost-scsi.c
> > +++ b/hw/scsi/vhost-scsi.c
> > @@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
> > **errp)
> >      s->dev.backend_features = 0;
> >  
> >      ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> > -                         VHOST_BACKEND_TYPE_KERNEL, true);
> > +                         VHOST_BACKEND_TYPE_KERNEL);
> >      if (ret < 0) {
> >          error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> >                     strerror(-ret));
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 5a12861..ce33e04 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct
> > vhost_virtqueue *vq)
> >  }
> >  
> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > -                   VhostBackendType backend_type, bool force)
> > +                   VhostBackendType backend_type)
> >  {
> >      uint64_t features;
> >      int i, r;
> > @@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >      hdev->started = false;
> >      hdev->memory_changed = false;
> >      memory_listener_register(&hdev->memory_listener, &address_space_memory);
> > -    hdev->force = force;
> >      return 0;
> >  fail_vq:
> >      while (--i >= 0) {
> > @@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice
> > *vdev)
> >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >  
> >      return !k->query_guest_notifiers ||
> > -           k->query_guest_notifiers(qbus->parent) ||
> > -           hdev->force;
> > +           k->query_guest_notifiers(qbus->parent);
> >  }
> >  
> >  /* Stop processing guest IO notifications in qemu.
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index d5593d1..27eae3e 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -46,7 +46,6 @@ struct vhost_dev {
> >      vhost_log_chunk_t *log;
> >      unsigned long long log_size;
> >      Error *migration_blocker;
> > -    bool force;
> >      bool memory_changed;
> >      hwaddr mem_changed_start_addr;
> >      hwaddr mem_changed_end_addr;
> > @@ -55,7 +54,7 @@ struct vhost_dev {
> >  };
> >  
> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > -                   VhostBackendType backend_type, bool force);
> > +                   VhostBackendType backend_type);
> >  void vhost_dev_cleanup(struct vhost_dev *hdev);
> >  bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
> >  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > index b1c18a3..966a945 100644
> > --- a/include/net/vhost_net.h
> > +++ b/include/net/vhost_net.h
> > @@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
> >      VhostBackendType backend_type;
> >      NetClientState *net_backend;
> >      void *opaque;
> > -    bool force;
> >  } VhostNetOptions;
> >  
> >  struct vhost_net *vhost_net_init(VhostNetOptions *options);
> > diff --git a/net/tap.c b/net/tap.c
> > index 968df46..b9002f7 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions
> > *tap, NetClientState *peer,
> >          }
> >      }
> >  
> > -    if (tap->has_vhost ? tap->vhost :
> > -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> > +    if (tap->has_vhost ? tap->vhost : vhostfdname) {
> >          VhostNetOptions options;
> >  
> >          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
> >          options.net_backend = &s->nc;
> > -        options.force = tap->has_vhostforce && tap->vhostforce;
> >  
> >          if (tap->has_vhostfd || tap->has_vhostfds) {
> >              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 24e050c..9966de5 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
> >      options.backend_type = VHOST_BACKEND_TYPE_USER;
> >      options.net_backend = &s->nc;
> >      options.opaque = s->chr;
> > -    options.force = s->vhostforce;
> >  
> >      s->vhost_net = vhost_net_init(&options);
> >  
> > --
> > 1.8.3.1
> > 
> > 
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-27 11:57     ` Michael S. Tsirkin
@ 2015-05-28  3:21       ` Jason Wang
  2015-05-28  3:36         ` Jason Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2015-05-28  3:21 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Pankaj Gupta, qemu-devel, stefanha, aliguori



On 05/27/2015 07:57 PM, Michael S. Tsirkin wrote:
> On Wed, May 27, 2015 at 04:45:34PM +0800, Jason Wang wrote:
>> > 
>> > 
>> > On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
>>> > > Ping.
>>> > >
>>> > > Can I get any suggestions on this patch.
>>> > >
>>> > > Best regards,
>>> > > Pankaj
>>> > >
>>>> > >> vhostforce was added to enable vhost when
>>>> > >>  guest don't have MSI-X support.
>>>> > >>  Now, we have scenarios like DPDK in Guest which dont use
>>>> > >>  interrupts and still use vhost. Also, performance of guests
>>>> > >>  without MSI-X support is getting less popular.
>>>> > >>
>>>> > >>  Its OK to remove this extra option and enable vhost
>>>> > >>  on the basis of vhost=ON/OFF.
>>>> > >>  Done basic testing with vhost on/off for latest guests
>>>> > >>  and old guests(non-msix).
>>>> > >>
>>>> > >> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>> > 
>> > Looks good. Two questions:
>> > 
>> > - Did libvirt use this? if not, we may want to drop vhostfore option
>> > completely.
> Yes, it did.
>

For vhost-user, vhostforce is mandatory. But how about tap? Looks like
it was not used, and I could not even find any option in
http://libvirt.org/formatdomain.html.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-28  3:21       ` Jason Wang
@ 2015-05-28  3:36         ` Jason Wang
  2015-05-28  6:28           ` Michal Privoznik
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2015-05-28  3:36 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Michal Privoznik, Pankaj Gupta, qemu-devel, aliguori



On 05/28/2015 11:21 AM, Jason Wang wrote:
>
> On 05/27/2015 07:57 PM, Michael S. Tsirkin wrote:
>> On Wed, May 27, 2015 at 04:45:34PM +0800, Jason Wang wrote:
>>>>
>>>> On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
>>>>>> Ping.
>>>>>>
>>>>>> Can I get any suggestions on this patch.
>>>>>>
>>>>>> Best regards,
>>>>>> Pankaj
>>>>>>
>>>>>>>> vhostforce was added to enable vhost when
>>>>>>>>  guest don't have MSI-X support.
>>>>>>>>  Now, we have scenarios like DPDK in Guest which dont use
>>>>>>>>  interrupts and still use vhost. Also, performance of guests
>>>>>>>>  without MSI-X support is getting less popular.
>>>>>>>>
>>>>>>>>  Its OK to remove this extra option and enable vhost
>>>>>>>>  on the basis of vhost=ON/OFF.
>>>>>>>>  Done basic testing with vhost on/off for latest guests
>>>>>>>>  and old guests(non-msix).
>>>>>>>>
>>>>>>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>>> Looks good. Two questions:
>>>>
>>>> - Did libvirt use this? if not, we may want to drop vhostfore option
>>>> completely.
>> Yes, it did.
>>
> For vhost-user, vhostforce is mandatory. But how about tap? Looks like
> it was not used, and I could not even find any option in
> http://libvirt.org/formatdomain.html.
>

CC Michal for the answer.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-28  3:36         ` Jason Wang
@ 2015-05-28  6:28           ` Michal Privoznik
  2015-05-28  6:43             ` Jason Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Michal Privoznik @ 2015-05-28  6:28 UTC (permalink / raw)
  To: Jason Wang, Michael S. Tsirkin; +Cc: Pankaj Gupta, qemu-devel, aliguori

On 28.05.2015 05:36, Jason Wang wrote:
> 
> 
> On 05/28/2015 11:21 AM, Jason Wang wrote:
>>
>> On 05/27/2015 07:57 PM, Michael S. Tsirkin wrote:
>>> On Wed, May 27, 2015 at 04:45:34PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
>>>>>>> Ping.
>>>>>>>
>>>>>>> Can I get any suggestions on this patch.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Pankaj
>>>>>>>
>>>>>>>>> vhostforce was added to enable vhost when
>>>>>>>>>  guest don't have MSI-X support.
>>>>>>>>>  Now, we have scenarios like DPDK in Guest which dont use
>>>>>>>>>  interrupts and still use vhost. Also, performance of guests
>>>>>>>>>  without MSI-X support is getting less popular.
>>>>>>>>>
>>>>>>>>>  Its OK to remove this extra option and enable vhost
>>>>>>>>>  on the basis of vhost=ON/OFF.
>>>>>>>>>  Done basic testing with vhost on/off for latest guests
>>>>>>>>>  and old guests(non-msix).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>>>> Looks good. Two questions:
>>>>>
>>>>> - Did libvirt use this? if not, we may want to drop vhostfore option
>>>>> completely.
>>> Yes, it did.
>>>
>> For vhost-user, vhostforce is mandatory. But how about tap? Looks like
>> it was not used, and I could not even find any option in
>> http://libvirt.org/formatdomain.html.
>>
> 
> CC Michal for the answer.
> 

No, vhostforce is not used by libvirt at all. Which brings up
interesting question: for vhost-user libvirt constructs merely the
follwing cmd line:

-chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \
-netdev type=vhost-user,id=hostnet0,chardev=charnet0 \

How does vhostforce fit in? I suppose the cmd line is working even
without libvirt passing vhostforce. Should it do so?

Michal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-28  6:28           ` Michal Privoznik
@ 2015-05-28  6:43             ` Jason Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2015-05-28  6:43 UTC (permalink / raw)
  To: Michal Privoznik, Michael S. Tsirkin; +Cc: Pankaj Gupta, qemu-devel, aliguori



On 05/28/2015 02:28 PM, Michal Privoznik wrote:
> On 28.05.2015 05:36, Jason Wang wrote:
>>
>> On 05/28/2015 11:21 AM, Jason Wang wrote:
>>> On 05/27/2015 07:57 PM, Michael S. Tsirkin wrote:
>>>> On Wed, May 27, 2015 at 04:45:34PM +0800, Jason Wang wrote:
>>>>>> On 05/27/2015 02:26 PM, Pankaj Gupta wrote:
>>>>>>>> Ping.
>>>>>>>>
>>>>>>>> Can I get any suggestions on this patch.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Pankaj
>>>>>>>>
>>>>>>>>>> vhostforce was added to enable vhost when
>>>>>>>>>>  guest don't have MSI-X support.
>>>>>>>>>>  Now, we have scenarios like DPDK in Guest which dont use
>>>>>>>>>>  interrupts and still use vhost. Also, performance of guests
>>>>>>>>>>  without MSI-X support is getting less popular.
>>>>>>>>>>
>>>>>>>>>>  Its OK to remove this extra option and enable vhost
>>>>>>>>>>  on the basis of vhost=ON/OFF.
>>>>>>>>>>  Done basic testing with vhost on/off for latest guests
>>>>>>>>>>  and old guests(non-msix).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>>>>> Looks good. Two questions:
>>>>>>
>>>>>> - Did libvirt use this? if not, we may want to drop vhostfore option
>>>>>> completely.
>>>> Yes, it did.
>>>>
>>> For vhost-user, vhostforce is mandatory. But how about tap? Looks like
>>> it was not used, and I could not even find any option in
>>> http://libvirt.org/formatdomain.html.
>>>
>> CC Michal for the answer.
>>
> No, vhostforce is not used by libvirt at all. Which brings up
> interesting question: for vhost-user libvirt constructs merely the
> follwing cmd line:
>
> -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \
> -netdev type=vhost-user,id=hostnet0,chardev=charnet0 \
>
> How does vhostforce fit in? I suppose the cmd line is working even
> without libvirt passing vhostforce. Should it do so?

Without this patch,  it works with normal virtio-net drivers but may not
work for guest pmd. Since pmd does not use MSI-X interrupts, so qemu
won't start vhost_net. So libvirt needs vhostforce for vhost-user.

With this patch, libvirt does not need to care about this.

>
> Michal
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter
  2015-05-27 12:00   ` Michael S. Tsirkin
@ 2015-05-29  4:59     ` Pankaj Gupta
  0 siblings, 0 replies; 10+ messages in thread
From: Pankaj Gupta @ 2015-05-29  4:59 UTC (permalink / raw)
  To: Michael S. Tsirkin, jasowang
  Cc: Michal Privoznik, qemu-devel, stefanha, aliguori


> > 
> > Ping.
> > 
> > Can I get any suggestions on this patch.
> > 
> > Best regards,
> > Pankaj
> 
> 
> 
> > > 
> > > vhostforce was added to enable vhost when
> > >  guest don't have MSI-X support.
> > >  Now, we have scenarios like DPDK in Guest which dont use
> > >  interrupts and still use vhost. Also, performance of guests
> > >  without MSI-X support is getting less popular.
> > > 
> > >  Its OK to remove this extra option and enable vhost
> > >  on the basis of vhost=ON/OFF.
> > >  Done basic testing with vhost on/off for latest guests
> > >  and old guests(non-msix).
> > > 
> > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> 
> I think this silently changes the command line semantics:
> previously vhostforce=on implies vhost=on, with this patch
> it does not.

Yes, we are trying to eliminate 'vhostforce'. As we already have vhost='ON/OFF'.

In case of 'virtio-net', with vhost= 'ON', without guest support of MSI-X we will fall
back to QEMU. If we use vhostforce='ON', we will use 'vhost' even it has to follow a long path
again via QEMU->vhost. also we don't have support of KVM level triggered irqfd.

In case 'vhost-user' is used, we are hard-coding 'vhostforce' to 'true' because we want 
vhost(vhost-user) to run. We are also removing this as it alos checks 'vhost_dev_query'.

Any use-case where we want to forcefully use vhost even we are falling back to userspace Virtio-net?

One use-case I can think for 'vhost-user' without MSI-X support e.g iPXE boot. Here we want to use vhost-user
for every/any condition, because we don't have any alternative. 

Any suggestions?  

> 
> > > ---
> > >  hw/net/vhost_net.c        | 2 +-
> > >  hw/scsi/vhost-scsi.c      | 2 +-
> > >  hw/virtio/vhost.c         | 6 ++----
> > >  include/hw/virtio/vhost.h | 3 +--
> > >  include/net/vhost_net.h   | 1 -
> > >  net/tap.c                 | 4 +---
> > >  net/vhost-user.c          | 1 -
> > >  7 files changed, 6 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 4e3a061..7139b9f 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -159,7 +159,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > > *options)
> > >      net->dev.vqs = net->vqs;
> > >  
> > >      r = vhost_dev_init(&net->dev, options->opaque,
> > > -                       options->backend_type, options->force);
> > > +                       options->backend_type);
> > >      if (r < 0) {
> > >          goto fail;
> > >      }
> > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > > index 618b0af..b15390e 100644
> > > --- a/hw/scsi/vhost-scsi.c
> > > +++ b/hw/scsi/vhost-scsi.c
> > > @@ -246,7 +246,7 @@ static void vhost_scsi_realize(DeviceState *dev,
> > > Error
> > > **errp)
> > >      s->dev.backend_features = 0;
> > >  
> > >      ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> > > -                         VHOST_BACKEND_TYPE_KERNEL, true);
> > > +                         VHOST_BACKEND_TYPE_KERNEL);
> > >      if (ret < 0) {
> > >          error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> > >                     strerror(-ret));
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 5a12861..ce33e04 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -811,7 +811,7 @@ static void vhost_virtqueue_cleanup(struct
> > > vhost_virtqueue *vq)
> > >  }
> > >  
> > >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > > -                   VhostBackendType backend_type, bool force)
> > > +                   VhostBackendType backend_type)
> > >  {
> > >      uint64_t features;
> > >      int i, r;
> > > @@ -874,7 +874,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> > > *opaque,
> > >      hdev->started = false;
> > >      hdev->memory_changed = false;
> > >      memory_listener_register(&hdev->memory_listener,
> > >      &address_space_memory);
> > > -    hdev->force = force;
> > >      return 0;
> > >  fail_vq:
> > >      while (--i >= 0) {
> > > @@ -909,8 +908,7 @@ bool vhost_dev_query(struct vhost_dev *hdev,
> > > VirtIODevice
> > > *vdev)
> > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> > >  
> > >      return !k->query_guest_notifiers ||
> > > -           k->query_guest_notifiers(qbus->parent) ||
> > > -           hdev->force;
> > > +           k->query_guest_notifiers(qbus->parent);
> > >  }
> > >  
> > >  /* Stop processing guest IO notifications in qemu.
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index d5593d1..27eae3e 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -46,7 +46,6 @@ struct vhost_dev {
> > >      vhost_log_chunk_t *log;
> > >      unsigned long long log_size;
> > >      Error *migration_blocker;
> > > -    bool force;
> > >      bool memory_changed;
> > >      hwaddr mem_changed_start_addr;
> > >      hwaddr mem_changed_end_addr;
> > > @@ -55,7 +54,7 @@ struct vhost_dev {
> > >  };
> > >  
> > >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > > -                   VhostBackendType backend_type, bool force);
> > > +                   VhostBackendType backend_type);
> > >  void vhost_dev_cleanup(struct vhost_dev *hdev);
> > >  bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
> > >  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> > > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > > index b1c18a3..966a945 100644
> > > --- a/include/net/vhost_net.h
> > > +++ b/include/net/vhost_net.h
> > > @@ -11,7 +11,6 @@ typedef struct VhostNetOptions {
> > >      VhostBackendType backend_type;
> > >      NetClientState *net_backend;
> > >      void *opaque;
> > > -    bool force;
> > >  } VhostNetOptions;
> > >  
> > >  struct vhost_net *vhost_net_init(VhostNetOptions *options);
> > > diff --git a/net/tap.c b/net/tap.c
> > > index 968df46..b9002f7 100644
> > > --- a/net/tap.c
> > > +++ b/net/tap.c
> > > @@ -635,13 +635,11 @@ static int net_init_tap_one(const NetdevTapOptions
> > > *tap, NetClientState *peer,
> > >          }
> > >      }
> > >  
> > > -    if (tap->has_vhost ? tap->vhost :
> > > -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> > > +    if (tap->has_vhost ? tap->vhost : vhostfdname) {
> > >          VhostNetOptions options;
> > >  
> > >          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
> > >          options.net_backend = &s->nc;
> > > -        options.force = tap->has_vhostforce && tap->vhostforce;
> > >  
> > >          if (tap->has_vhostfd || tap->has_vhostfds) {
> > >              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > index 24e050c..9966de5 100644
> > > --- a/net/vhost-user.c
> > > +++ b/net/vhost-user.c
> > > @@ -51,7 +51,6 @@ static int vhost_user_start(VhostUserState *s)
> > >      options.backend_type = VHOST_BACKEND_TYPE_USER;
> > >      options.net_backend = &s->nc;
> > >      options.opaque = s->chr;
> > > -    options.force = s->vhostforce;
> > >  
> > >      s->vhost_net = vhost_net_init(&options);
> > >  
> > > --
> > > 1.8.3.1
> > > 
> > > 
> > > 
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-05-29  5:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-09  6:00 [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter Pankaj Gupta
2015-05-27  6:26 ` Pankaj Gupta
2015-05-27  8:45   ` Jason Wang
2015-05-27 11:57     ` Michael S. Tsirkin
2015-05-28  3:21       ` Jason Wang
2015-05-28  3:36         ` Jason Wang
2015-05-28  6:28           ` Michal Privoznik
2015-05-28  6:43             ` Jason Wang
2015-05-27 12:00   ` Michael S. Tsirkin
2015-05-29  4:59     ` Pankaj Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).