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

[Raphael Norwitz]

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