* [PATCH] vhost-blk: set features before setting inflight feature @ 2020-09-10 13:48 Jin Yu 2020-09-15 1:25 ` Raphael Norwitz 2020-11-03 12:36 ` [PATCH v2] " Jin Yu 0 siblings, 2 replies; 7+ messages in thread From: Jin Yu @ 2020-09-10 13:48 UTC (permalink / raw) To: mst, raphael.norwitz, kwolf, mreitz; +Cc: Jin Yu, qemu-devel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-blk: set features before setting inflight feature 2020-09-10 13:48 [PATCH] vhost-blk: set features before setting inflight feature Jin Yu @ 2020-09-15 1:25 ` Raphael Norwitz 2020-09-15 2:52 ` Yu, Jin 2020-11-03 12:36 ` [PATCH v2] " Jin Yu 1 sibling, 1 reply; 7+ messages in thread From: Raphael Norwitz @ 2020-09-15 1:25 UTC (permalink / raw) To: Jin Yu; +Cc: Kevin Wolf, QEMU, Raphael Norwitz, Max Reitz, Michael S. Tsirkin 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] vhost-blk: set features before setting inflight feature 2020-09-15 1:25 ` Raphael Norwitz @ 2020-09-15 2:52 ` Yu, Jin 2020-09-21 23:03 ` Raphael Norwitz 0 siblings, 1 reply; 7+ messages in thread From: Yu, Jin @ 2020-09-15 2:52 UTC (permalink / raw) To: Raphael Norwitz Cc: Kevin Wolf, QEMU, Raphael Norwitz, Max Reitz, Michael S. Tsirkin > -----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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-blk: set features before setting inflight feature 2020-09-15 2:52 ` Yu, Jin @ 2020-09-21 23:03 ` Raphael Norwitz 2020-09-22 7:02 ` Yu, Jin 0 siblings, 1 reply; 7+ messages in thread From: Raphael Norwitz @ 2020-09-21 23:03 UTC (permalink / raw) To: Yu, Jin; +Cc: Kevin Wolf, QEMU, Raphael Norwitz, Max Reitz, Michael S. Tsirkin 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 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] vhost-blk: set features before setting inflight feature 2020-09-21 23:03 ` Raphael Norwitz @ 2020-09-22 7:02 ` Yu, Jin 2020-10-02 3:02 ` Raphael Norwitz 0 siblings, 1 reply; 7+ messages in thread From: Yu, Jin @ 2020-09-22 7:02 UTC (permalink / raw) To: Raphael Norwitz Cc: Kevin Wolf, QEMU, Raphael Norwitz, Max Reitz, Michael S. Tsirkin > -----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 > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-blk: set features before setting inflight feature 2020-09-22 7:02 ` Yu, Jin @ 2020-10-02 3:02 ` Raphael Norwitz 0 siblings, 0 replies; 7+ messages in thread From: Raphael Norwitz @ 2020-10-02 3:02 UTC (permalink / raw) To: Yu, Jin; +Cc: Kevin Wolf, QEMU, Raphael Norwitz, Max Reitz, Michael S. Tsirkin 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] vhost-blk: set features before setting inflight feature 2020-09-10 13:48 [PATCH] vhost-blk: set features before setting inflight feature Jin Yu 2020-09-15 1:25 ` Raphael Norwitz @ 2020-11-03 12:36 ` Jin Yu 1 sibling, 0 replies; 7+ messages in thread From: Jin Yu @ 2020-11-03 12:36 UTC (permalink / raw) To: raphael.norwitz, mst, kwolf, mreitz; +Cc: Jin Yu, qemu-devel, qemu-block 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> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com> --- v2: * Fixed the segfault. --- hw/block/vhost-user-blk.c | 6 ++++++ hw/virtio/vhost.c | 20 ++++++++++++++++++++ include/hw/virtio/vhost.h | 1 + 3 files changed, 27 insertions(+) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 39aec42dae..db079a89c0 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, vdev); + 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..6ffbfbfb9e 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1616,6 +1616,26 @@ int vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f) return 0; } +int vhost_dev_prepare_inflight(struct vhost_dev *hdev, VirtIODevice *vdev) +{ + int r; + + if (hdev->vhost_ops->vhost_get_inflight_fd == NULL || + hdev->vhost_ops->vhost_set_inflight_fd == NULL) { + return 0; + } + + hdev->vdev = vdev; + + 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..d25f0947f7 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, VirtIODevice *vdev); 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-03 5:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-10 13:48 [PATCH] vhost-blk: set features before setting inflight feature Jin Yu 2020-09-15 1:25 ` Raphael Norwitz 2020-09-15 2:52 ` Yu, Jin 2020-09-21 23:03 ` Raphael Norwitz 2020-09-22 7:02 ` Yu, Jin 2020-10-02 3:02 ` Raphael Norwitz 2020-11-03 12:36 ` [PATCH v2] " Jin Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).