1. Patchew
  2. QEMU
  3. View series

[PATCH] vhost-blk: set features before setting inflight feature

Jin Yu posted 1 patch 3 years, 7 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200910134851.7817-1-jin.yu@intel.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Max Reitz <mreitz@redhat.com>, Raphael Norwitz <raphael.norwitz@nutanix.com>
hw/block/vhost-user-blk.c |  6 ++++++
hw/virtio/vhost.c         | 18 ++++++++++++++++++
include/hw/virtio/vhost.h |  1 +
3 files changed, 25 insertions(+)
[PATCH] vhost-blk: set features before setting inflight feature
Posted by Jin Yu 3 years, 7 months ago 
Virtqueue has split and packed, so before setting inflight,
you need to inform the back-end virtqueue format.

Signed-off-by: Jin Yu <jin.yu@intel.com>
---
 hw/block/vhost-user-blk.c |  6 ++++++
 hw/virtio/vhost.c         | 18 ++++++++++++++++++
 include/hw/virtio/vhost.h |  1 +
 3 files changed, 25 insertions(+)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 39aec42dae..9e0e9ebec0 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
 
     s->dev.acked_features = vdev->guest_features;
 
+    ret = vhost_dev_prepare_inflight(&s->dev);
+    if (ret < 0) {
+        error_report("Error set inflight format: %d", -ret);
+        goto err_guest_notifiers;
+    }
+
     if (!s->inflight->addr) {
         ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
         if (ret < 0) {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1a1384e7a6..4027c11886 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f)
     return 0;
 }
 
+int vhost_dev_prepare_inflight(struct vhost_dev *hdev)
+{
+    int r;
+ 
+    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
+        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
+        return 0;
+    }
+ 
+    r = vhost_dev_set_features(hdev, hdev->log_enabled);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
+        return r;
+    }
+
+    return 0;
+}
+
 int vhost_dev_set_inflight(struct vhost_dev *dev,
                            struct vhost_inflight *inflight)
 {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 767a95ec0b..4e2fc75528 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
 void vhost_dev_free_inflight(struct vhost_inflight *inflight);
 void vhost_dev_save_inflight(struct vhost_inflight *inflight, QEMUFile *f);
 int vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f);
+int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
 int vhost_dev_set_inflight(struct vhost_dev *dev,
                            struct vhost_inflight *inflight);
 int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
-- 
2.17.2
Re: [PATCH] vhost-blk: set features before setting inflight feature
Posted by Raphael Norwitz 3 years, 7 months ago 
Backends already receive the format in vhost_dev_start before the
memory tables are set or any of the virtqueues are started. Can you
elaborate on why you need to know the virtqueue format before setting
the inflight FD?

On Thu, Sep 10, 2020 at 2:15 AM Jin Yu <jin.yu@intel.com> wrote:
>
> Virtqueue has split and packed, so before setting inflight,
> you need to inform the back-end virtqueue format.
>
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> ---
>  hw/block/vhost-user-blk.c |  6 ++++++
>  hw/virtio/vhost.c         | 18 ++++++++++++++++++
>  include/hw/virtio/vhost.h |  1 +
>  3 files changed, 25 insertions(+)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 39aec42dae..9e0e9ebec0 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
>
>      s->dev.acked_features = vdev->guest_features;
>
> +    ret = vhost_dev_prepare_inflight(&s->dev);
> +    if (ret < 0) {
> +        error_report("Error set inflight format: %d", -ret);
> +        goto err_guest_notifiers;
> +    }
> +
>      if (!s->inflight->addr) {
>          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
>          if (ret < 0) {
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a6..4027c11886 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f)
>      return 0;
>  }
>
> +int vhost_dev_prepare_inflight(struct vhost_dev *hdev)
> +{
> +    int r;
> +
> +    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> +        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> +        return 0;
> +    }
> +
> +    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> +    if (r < 0) {
> +        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> +        return r;
> +    }
> +
> +    return 0;
> +}
> +
>  int vhost_dev_set_inflight(struct vhost_dev *dev,
>                             struct vhost_inflight *inflight)
>  {
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 767a95ec0b..4e2fc75528 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
>  void vhost_dev_free_inflight(struct vhost_inflight *inflight);
>  void vhost_dev_save_inflight(struct vhost_inflight *inflight, QEMUFile *f);
>  int vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f);
> +int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
>  int vhost_dev_set_inflight(struct vhost_dev *dev,
>                             struct vhost_inflight *inflight);
>  int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> --
> 2.17.2
>
>
RE: [PATCH] vhost-blk: set features before setting inflight feature
Posted by Yu, Jin 3 years, 7 months ago 
> -----Original Message-----
> From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> Sent: Tuesday, September 15, 2020 9:25 AM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] vhost-blk: set features before setting inflight feature
> 
> Backends already receive the format in vhost_dev_start before the memory
> tables are set or any of the virtqueues are started. Can you elaborate on why
> you need to know the virtqueue format before setting the inflight FD?
> 
First, when the backend receives the get_inflight_fd sent by QEMU, it needs to allocate vq's inflight memory,
and it needs to know the format of vq.
Second, when the backend reconnects to QEMU, QEMU sends set_inflight_fd, and the backend restores the inflight memory of vq. 
It also needs to know the format of vq. 
Thanks.
> On Thu, Sep 10, 2020 at 2:15 AM Jin Yu <jin.yu@intel.com> wrote:
> >
> > Virtqueue has split and packed, so before setting inflight, you need
> > to inform the back-end virtqueue format.
> >
> > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > ---
> >  hw/block/vhost-user-blk.c |  6 ++++++
> >  hw/virtio/vhost.c         | 18 ++++++++++++++++++
> >  include/hw/virtio/vhost.h |  1 +
> >  3 files changed, 25 insertions(+)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 39aec42dae..9e0e9ebec0 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice
> > *vdev)
> >
> >      s->dev.acked_features = vdev->guest_features;
> >
> > +    ret = vhost_dev_prepare_inflight(&s->dev);
> > +    if (ret < 0) {
> > +        error_report("Error set inflight format: %d", -ret);
> > +        goto err_guest_notifiers;
> > +    }
> > +
> >      if (!s->inflight->addr) {
> >          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
> >          if (ret < 0) {
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > 1a1384e7a6..4027c11886 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct vhost_inflight
> *inflight, QEMUFile *f)
> >      return 0;
> >  }
> >
> > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev) {
> > +    int r;
> > +
> > +    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> > +        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> > +        return 0;
> > +    }
> > +
> > +    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> > +    if (r < 0) {
> > +        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> > +        return r;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> >                             struct vhost_inflight *inflight)  { diff
> > --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index
> > 767a95ec0b..4e2fc75528 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct
> > vhost_inflight *inflight);  void vhost_dev_free_inflight(struct
> > vhost_inflight *inflight);  void vhost_dev_save_inflight(struct
> > vhost_inflight *inflight, QEMUFile *f);  int
> > vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f);
> > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
> >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> >                             struct vhost_inflight *inflight);  int
> > vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > --
> > 2.17.2
> >
> >
Re: [PATCH] vhost-blk: set features before setting inflight feature
Posted by Raphael Norwitz 3 years, 7 months ago 
I see your point - all the open source backends I could find which
support VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD rely on knowing the vq
type to allocate the fd.

That said, it looks like dpdk supports both
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD and packed vqs without needing
such an API https://github.com/DPDK/dpdk/blob/main/lib/librte_vhost/vhost_user.c#L1509.
I'm not sure exactly how the VQ state is sent to DPDK before the
inflight fd negotiation, but ideally vhost-user-blk could be made to
work the same way. Maybe someone with more vhost-net and dpdk
knowledge could chime in on how vhost-net backends do it?

On Mon, Sep 14, 2020 at 10:52 PM Yu, Jin <jin.yu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> > Sent: Tuesday, September 15, 2020 9:25 AM
> > To: Yu, Jin <jin.yu@intel.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> > <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> > Subject: Re: [PATCH] vhost-blk: set features before setting inflight feature
> >
> > Backends already receive the format in vhost_dev_start before the memory
> > tables are set or any of the virtqueues are started. Can you elaborate on why
> > you need to know the virtqueue format before setting the inflight FD?
> >
> First, when the backend receives the get_inflight_fd sent by QEMU, it needs to allocate vq's inflight memory,
> and it needs to know the format of vq.
> Second, when the backend reconnects to QEMU, QEMU sends set_inflight_fd, and the backend restores the inflight memory of vq.
> It also needs to know the format of vq.
> Thanks.
> > On Thu, Sep 10, 2020 at 2:15 AM Jin Yu <jin.yu@intel.com> wrote:
> > >
> > > Virtqueue has split and packed, so before setting inflight, you need
> > > to inform the back-end virtqueue format.
> > >
> > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > ---
> > >  hw/block/vhost-user-blk.c |  6 ++++++
> > >  hw/virtio/vhost.c         | 18 ++++++++++++++++++
> > >  include/hw/virtio/vhost.h |  1 +
> > >  3 files changed, 25 insertions(+)
> > >
> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > index 39aec42dae..9e0e9ebec0 100644
> > > --- a/hw/block/vhost-user-blk.c
> > > +++ b/hw/block/vhost-user-blk.c
> > > @@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice
> > > *vdev)
> > >
> > >      s->dev.acked_features = vdev->guest_features;
> > >
> > > +    ret = vhost_dev_prepare_inflight(&s->dev);
> > > +    if (ret < 0) {
> > > +        error_report("Error set inflight format: %d", -ret);
> > > +        goto err_guest_notifiers;
> > > +    }
> > > +
> > >      if (!s->inflight->addr) {
> > >          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
> > >          if (ret < 0) {
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > > 1a1384e7a6..4027c11886 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct vhost_inflight
> > *inflight, QEMUFile *f)
> > >      return 0;
> > >  }
> > >
> > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev) {
> > > +    int r;
> > > +
> > > +    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> > > +        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> > > +    if (r < 0) {
> > > +        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> > > +        return r;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > >                             struct vhost_inflight *inflight)  { diff
> > > --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index
> > > 767a95ec0b..4e2fc75528 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct
> > > vhost_inflight *inflight);  void vhost_dev_free_inflight(struct
> > > vhost_inflight *inflight);  void vhost_dev_save_inflight(struct
> > > vhost_inflight *inflight, QEMUFile *f);  int
> > > vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f);
> > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
> > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > >                             struct vhost_inflight *inflight);  int
> > > vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > > --
> > > 2.17.2
> > >
> > >
RE: [PATCH] vhost-blk: set features before setting inflight feature
Posted by Yu, Jin 3 years, 7 months ago 
> -----Original Message-----
> From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> Sent: Tuesday, September 22, 2020 7:03 AM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] vhost-blk: set features before setting inflight feature
> 
> I see your point - all the open source backends I could find which support
> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD rely on knowing the vq type
> to allocate the fd.
> 
> That said, it looks like dpdk supports both
> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD and packed vqs without
> needing such an API
> https://github.com/DPDK/dpdk/blob/main/lib/librte_vhost/vhost_user.c#L1
> 509.
> I'm not sure exactly how the VQ state is sent to DPDK before the inflight fd
> negotiation, but ideally vhost-user-blk could be made to work the same way.
> Maybe someone with more vhost-net and dpdk knowledge could chime in on
> how vhost-net backends do it?
>
I checked the code of vhost-net in QEMU, it did not use the inflight feature, 
which should be different from storage, after all, the network can lose packets
and retransmit.

Of course, as you said, we need an expert familiar with vhost-net and dpdk.

Thanks
> On Mon, Sep 14, 2020 at 10:52 PM Yu, Jin <jin.yu@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> > > Sent: Tuesday, September 15, 2020 9:25 AM
> > > To: Yu, Jin <jin.yu@intel.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> > > <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > > Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> > > Subject: Re: [PATCH] vhost-blk: set features before setting inflight
> > > feature
> > >
> > > Backends already receive the format in vhost_dev_start before the
> > > memory tables are set or any of the virtqueues are started. Can you
> > > elaborate on why you need to know the virtqueue format before setting
> the inflight FD?
> > >
> > First, when the backend receives the get_inflight_fd sent by QEMU, it
> > needs to allocate vq's inflight memory, and it needs to know the format of
> vq.
> > Second, when the backend reconnects to QEMU, QEMU sends
> set_inflight_fd, and the backend restores the inflight memory of vq.
> > It also needs to know the format of vq.
> > Thanks.
> > > On Thu, Sep 10, 2020 at 2:15 AM Jin Yu <jin.yu@intel.com> wrote:
> > > >
> > > > Virtqueue has split and packed, so before setting inflight, you
> > > > need to inform the back-end virtqueue format.
> > > >
> > > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > > ---
> > > >  hw/block/vhost-user-blk.c |  6 ++++++
> > > >  hw/virtio/vhost.c         | 18 ++++++++++++++++++
> > > >  include/hw/virtio/vhost.h |  1 +
> > > >  3 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > > index 39aec42dae..9e0e9ebec0 100644
> > > > --- a/hw/block/vhost-user-blk.c
> > > > +++ b/hw/block/vhost-user-blk.c
> > > > @@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice
> > > > *vdev)
> > > >
> > > >      s->dev.acked_features = vdev->guest_features;
> > > >
> > > > +    ret = vhost_dev_prepare_inflight(&s->dev);
> > > > +    if (ret < 0) {
> > > > +        error_report("Error set inflight format: %d", -ret);
> > > > +        goto err_guest_notifiers;
> > > > +    }
> > > > +
> > > >      if (!s->inflight->addr) {
> > > >          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
> > > >          if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > > > 1a1384e7a6..4027c11886 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct
> > > > vhost_inflight
> > > *inflight, QEMUFile *f)
> > > >      return 0;
> > > >  }
> > > >
> > > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev) {
> > > > +    int r;
> > > > +
> > > > +    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> > > > +        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> > > > +    if (r < 0) {
> > > > +        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> > > > +        return r;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > > >                             struct vhost_inflight *inflight)  {
> > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > index
> > > > 767a95ec0b..4e2fc75528 100644
> > > > --- a/include/hw/virtio/vhost.h
> > > > +++ b/include/hw/virtio/vhost.h
> > > > @@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct
> > > > vhost_inflight *inflight);  void vhost_dev_free_inflight(struct
> > > > vhost_inflight *inflight);  void vhost_dev_save_inflight(struct
> > > > vhost_inflight *inflight, QEMUFile *f);  int
> > > > vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile
> > > > *f);
> > > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
> > > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > > >                             struct vhost_inflight *inflight);  int
> > > > vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > > > --
> > > > 2.17.2
> > > >
> > > >
Re: [PATCH] vhost-blk: set features before setting inflight feature
Posted by Raphael Norwitz 3 years, 6 months ago 
I see you're right - or rather even if a vhost-net backend negotiates
the inflight feature, QEMU never sets/gets an inflight fd. That clears
up all my concerns - I support the change.

I don't like sending an additional VHOST_USER_SET_FEATURES message in
vhost_dev_start() right after we've sent one in
vhost_dev_prepare_inflight(), but I don't see a clean way around it.
We could add a flag in vhost_dev, set it in
vhost_dev_prepare_inflight() and then check and set it back in
vhost_dev_set_features(), but that seems quite ugly. Unless anyone can
think of a better option, I say we go with the patch as is.

Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

On Tue, Sep 22, 2020 at 3:03 AM Yu, Jin <jin.yu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> > Sent: Tuesday, September 22, 2020 7:03 AM
> > To: Yu, Jin <jin.yu@intel.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> > <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> > Subject: Re: [PATCH] vhost-blk: set features before setting inflight feature
> >
> > I see your point - all the open source backends I could find which support
> > VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD rely on knowing the vq type
> > to allocate the fd.
> >
> > That said, it looks like dpdk supports both
> > VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD and packed vqs without
> > needing such an API
> > https://github.com/DPDK/dpdk/blob/main/lib/librte_vhost/vhost_user.c#L1
> > 509.
> > I'm not sure exactly how the VQ state is sent to DPDK before the inflight fd
> > negotiation, but ideally vhost-user-blk could be made to work the same way.
> > Maybe someone with more vhost-net and dpdk knowledge could chime in on
> > how vhost-net backends do it?
> >
> I checked the code of vhost-net in QEMU, it did not use the inflight feature,
> which should be different from storage, after all, the network can lose packets
> and retransmit.
>
> Of course, as you said, we need an expert familiar with vhost-net and dpdk.
>
> Thanks
> > On Mon, Sep 14, 2020 at 10:52 PM Yu, Jin <jin.yu@intel.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> > > > Sent: Tuesday, September 15, 2020 9:25 AM
> > > > To: Yu, Jin <jin.yu@intel.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>; Raphael Norwitz
> > > > <raphael.norwitz@nutanix.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > > > Reitz <mreitz@redhat.com>; QEMU <qemu-devel@nongnu.org>
> > > > Subject: Re: [PATCH] vhost-blk: set features before setting inflight
> > > > feature
> > > >
> > > > Backends already receive the format in vhost_dev_start before the
> > > > memory tables are set or any of the virtqueues are started. Can you
> > > > elaborate on why you need to know the virtqueue format before setting
> > the inflight FD?
> > > >
> > > First, when the backend receives the get_inflight_fd sent by QEMU, it
> > > needs to allocate vq's inflight memory, and it needs to know the format of
> > vq.
> > > Second, when the backend reconnects to QEMU, QEMU sends
> > set_inflight_fd, and the backend restores the inflight memory of vq.
> > > It also needs to know the format of vq.
> > > Thanks.
> > > > On Thu, Sep 10, 2020 at 2:15 AM Jin Yu <jin.yu@intel.com> wrote:
> > > > >
> > > > > Virtqueue has split and packed, so before setting inflight, you
> > > > > need to inform the back-end virtqueue format.
> > > > >
> > > > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > > > ---
> > > > >  hw/block/vhost-user-blk.c |  6 ++++++
> > > > >  hw/virtio/vhost.c         | 18 ++++++++++++++++++
> > > > >  include/hw/virtio/vhost.h |  1 +
> > > > >  3 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > > > index 39aec42dae..9e0e9ebec0 100644
> > > > > --- a/hw/block/vhost-user-blk.c
> > > > > +++ b/hw/block/vhost-user-blk.c
> > > > > @@ -131,6 +131,12 @@ static int vhost_user_blk_start(VirtIODevice
> > > > > *vdev)
> > > > >
> > > > >      s->dev.acked_features = vdev->guest_features;
> > > > >
> > > > > +    ret = vhost_dev_prepare_inflight(&s->dev);
> > > > > +    if (ret < 0) {
> > > > > +        error_report("Error set inflight format: %d", -ret);
> > > > > +        goto err_guest_notifiers;
> > > > > +    }
> > > > > +
> > > > >      if (!s->inflight->addr) {
> > > > >          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
> > > > >          if (ret < 0) {
> > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > > > > 1a1384e7a6..4027c11886 100644
> > > > > --- a/hw/virtio/vhost.c
> > > > > +++ b/hw/virtio/vhost.c
> > > > > @@ -1616,6 +1616,24 @@ int vhost_dev_load_inflight(struct
> > > > > vhost_inflight
> > > > *inflight, QEMUFile *f)
> > > > >      return 0;
> > > > >  }
> > > > >
> > > > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev) {
> > > > > +    int r;
> > > > > +
> > > > > +    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> > > > > +        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> > > > > +        return 0;
> > > > > +    }
> > > > > +
> > > > > +    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> > > > > +    if (r < 0) {
> > > > > +        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> > > > > +        return r;
> > > > > +    }
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > > > >                             struct vhost_inflight *inflight)  {
> > > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > > index
> > > > > 767a95ec0b..4e2fc75528 100644
> > > > > --- a/include/hw/virtio/vhost.h
> > > > > +++ b/include/hw/virtio/vhost.h
> > > > > @@ -140,6 +140,7 @@ void vhost_dev_reset_inflight(struct
> > > > > vhost_inflight *inflight);  void vhost_dev_free_inflight(struct
> > > > > vhost_inflight *inflight);  void vhost_dev_save_inflight(struct
> > > > > vhost_inflight *inflight, QEMUFile *f);  int
> > > > > vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile
> > > > > *f);
> > > > > +int vhost_dev_prepare_inflight(struct vhost_dev *hdev);
> > > > >  int vhost_dev_set_inflight(struct vhost_dev *dev,
> > > > >                             struct vhost_inflight *inflight);  int
> > > > > vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > > > > --
> > > > > 2.17.2
> > > > >
> > > > >

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.