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.norw...@nutanix.com> On Tue, Sep 22, 2020 at 3:03 AM Yu, Jin <jin...@intel.com> wrote: > > > -----Original Message----- > > From: Raphael Norwitz <raphael.s.norw...@gmail.com> > > Sent: Tuesday, September 22, 2020 7:03 AM > > To: Yu, Jin <jin...@intel.com> > > Cc: Michael S. Tsirkin <m...@redhat.com>; Raphael Norwitz > > <raphael.norw...@nutanix.com>; Kevin Wolf <kw...@redhat.com>; Max > > Reitz <mre...@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...@intel.com> wrote: > > > > > > > -----Original Message----- > > > > From: Raphael Norwitz <raphael.s.norw...@gmail.com> > > > > Sent: Tuesday, September 15, 2020 9:25 AM > > > > To: Yu, Jin <jin...@intel.com> > > > > Cc: Michael S. Tsirkin <m...@redhat.com>; Raphael Norwitz > > > > <raphael.norw...@nutanix.com>; Kevin Wolf <kw...@redhat.com>; Max > > > > Reitz <mre...@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...@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...@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 > > > > > > > > > >
