Re: [Qemu-devel] [PATCH] net: Remove vhostforce option in addition to vhost parameter

[Michael S. Tsirkin]

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 <address@hidden>
> 
> 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
> >>
> >>
> >>