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

[Yu, Jin]

> -----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
> > > >
> > > >