Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

[Li Feng]

On Sun, Oct 8, 2023 at 4:51 PM Michael S. Tsirkin  wrote:
>
> On Sun, Oct 08, 2023 at 04:49:05PM +0800, Li Feng wrote:
> > On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
> >  wrote:
> > >
> > >
> > >
> > > > On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> > > >
> > > > Currently the get_inflight_fd will be sent every time the device is 
> > > > started, and
> > > > the backend will allocate shared memory to save the inflight state. If 
> > > > the
> > > > backend finds that it receives the second get_inflight_fd, it will 
> > > > release the
> > > > previous shared memory, which breaks inflight working logic.
> > > >
> > > > This patch is a preparation for the following patches.
> > >
> > > This looks identical to the v3 patch I reviewed? If I’ve missed something 
> > > can you please point it out?
> > Yes, nothing changed in this patch.
>
>
> Then you should include tags such as reviewed/acked by for previous
> version. if you drop tags you indicate to people they have to
> re-review.
> also, mentioning which patches changed in the cover letter is
> a courtesy to reviewers.
OK.

>
> > >
> > >
> > > > Signed-off-by: Li Feng 
> > > > ---
> > > > hw/scsi/vhost-scsi-common.c | 37 ++---
> > > > 1 file changed, 18 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > > index a06f01af26..a61cd0e907 100644
> > > > --- a/hw/scsi/vhost-scsi-common.c
> > > > +++ b/hw/scsi/vhost-scsi-common.c
> > > > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > >
> > > > vsc->dev.acked_features = vdev->guest_features;
> > > >
> > > > -assert(vsc->inflight == NULL);
> > > > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > > > -ret = vhost_dev_get_inflight(&vsc->dev,
> > > > - vs->conf.virtqueue_size,
> > > > - vsc->inflight);
> > > > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > > > if (ret < 0) {
> > > > -error_report("Error get inflight: %d", -ret);
> > > > +error_report("Error setting inflight format: %d", -ret);
> > > > goto err_guest_notifiers;
> > > > }
> > > >
> > > > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > > -if (ret < 0) {
> > > > -error_report("Error set inflight: %d", -ret);
> > > > -goto err_guest_notifiers;
> > > > +if (vsc->inflight) {
> > > > +if (!vsc->inflight->addr) {
> > > > +ret = vhost_dev_get_inflight(&vsc->dev,
> > > > +vs->conf.virtqueue_size,
> > > > +vsc->inflight);
> > > > +if (ret < 0) {
> > > > +error_report("Error getting inflight: %d", -ret);
> > > > +goto err_guest_notifiers;
> > > > +}
> > > > +}
> > > > +
> > > > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > > +if (ret < 0) {
> > > > +error_report("Error setting inflight: %d", -ret);
> > > > +goto err_guest_notifiers;
> > > > +}
> > > > }
> > > >
> > > > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > > > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > > return ret;
> > > >
> > > > err_guest_notifiers:
> > > > -g_free(vsc->inflight);
> > > > -vsc->inflight = NULL;
> > > > -
> > > > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > > err_host_notifiers:
> > > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > > > }
> > > > assert(ret >= 0);
> > > >
> > > > -if (vsc->inflight) {
> > > > -vhost_dev_free_inflight(vsc->inflight);
> > > > -g_free(vsc->inflight);
> > > > -vsc->inflight = NULL;
> > > > -}
> > > > -
> > > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > > }
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
>

Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

[Li Feng]

On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
 wrote:
>
>
>
> > On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> >
> > Currently the get_inflight_fd will be sent every time the device is 
> > started, and
> > the backend will allocate shared memory to save the inflight state. If the
> > backend finds that it receives the second get_inflight_fd, it will release 
> > the
> > previous shared memory, which breaks inflight working logic.
> >
> > This patch is a preparation for the following patches.
>
> This looks identical to the v3 patch I reviewed? If I’ve missed something can 
> you please point it out?
Yes, nothing changed in this patch.

>
>
> > Signed-off-by: Li Feng 
> > ---
> > hw/scsi/vhost-scsi-common.c | 37 ++---
> > 1 file changed, 18 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > index a06f01af26..a61cd0e907 100644
> > --- a/hw/scsi/vhost-scsi-common.c
> > +++ b/hw/scsi/vhost-scsi-common.c
> > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> >
> > vsc->dev.acked_features = vdev->guest_features;
> >
> > -assert(vsc->inflight == NULL);
> > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > -ret = vhost_dev_get_inflight(&vsc->dev,
> > - vs->conf.virtqueue_size,
> > - vsc->inflight);
> > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > if (ret < 0) {
> > -error_report("Error get inflight: %d", -ret);
> > +error_report("Error setting inflight format: %d", -ret);
> > goto err_guest_notifiers;
> > }
> >
> > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > -if (ret < 0) {
> > -error_report("Error set inflight: %d", -ret);
> > -goto err_guest_notifiers;
> > +if (vsc->inflight) {
> > +if (!vsc->inflight->addr) {
> > +ret = vhost_dev_get_inflight(&vsc->dev,
> > +vs->conf.virtqueue_size,
> > +vsc->inflight);
> > +if (ret < 0) {
> > +error_report("Error getting inflight: %d", -ret);
> > +goto err_guest_notifiers;
> > +}
> > +}
> > +
> > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > +if (ret < 0) {
> > +error_report("Error setting inflight: %d", -ret);
> > +goto err_guest_notifiers;
> > +}
> > }
> >
> > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > return ret;
> >
> > err_guest_notifiers:
> > -g_free(vsc->inflight);
> > -vsc->inflight = NULL;
> > -
> > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > err_host_notifiers:
> > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > }
> > assert(ret >= 0);
> >
> > -if (vsc->inflight) {
> > -vhost_dev_free_inflight(vsc->inflight);
> > -g_free(vsc->inflight);
> > -vsc->inflight = NULL;
> > -}
> > -
> > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > }
> >
> > --
> > 2.41.0
> >
>

Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

[Michael S. Tsirkin]

On Sun, Oct 08, 2023 at 04:49:05PM +0800, Li Feng wrote:
> On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
>  wrote:
> >
> >
> >
> > > On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> > >
> > > Currently the get_inflight_fd will be sent every time the device is 
> > > started, and
> > > the backend will allocate shared memory to save the inflight state. If the
> > > backend finds that it receives the second get_inflight_fd, it will 
> > > release the
> > > previous shared memory, which breaks inflight working logic.
> > >
> > > This patch is a preparation for the following patches.
> >
> > This looks identical to the v3 patch I reviewed? If I’ve missed something 
> > can you please point it out?
> Yes, nothing changed in this patch.


Then you should include tags such as reviewed/acked by for previous
version. if you drop tags you indicate to people they have to
re-review.
also, mentioning which patches changed in the cover letter is
a courtesy to reviewers.

> >
> >
> > > Signed-off-by: Li Feng 
> > > ---
> > > hw/scsi/vhost-scsi-common.c | 37 ++---
> > > 1 file changed, 18 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > index a06f01af26..a61cd0e907 100644
> > > --- a/hw/scsi/vhost-scsi-common.c
> > > +++ b/hw/scsi/vhost-scsi-common.c
> > > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > >
> > > vsc->dev.acked_features = vdev->guest_features;
> > >
> > > -assert(vsc->inflight == NULL);
> > > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > > -ret = vhost_dev_get_inflight(&vsc->dev,
> > > - vs->conf.virtqueue_size,
> > > - vsc->inflight);
> > > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > > if (ret < 0) {
> > > -error_report("Error get inflight: %d", -ret);
> > > +error_report("Error setting inflight format: %d", -ret);
> > > goto err_guest_notifiers;
> > > }
> > >
> > > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > -if (ret < 0) {
> > > -error_report("Error set inflight: %d", -ret);
> > > -goto err_guest_notifiers;
> > > +if (vsc->inflight) {
> > > +if (!vsc->inflight->addr) {
> > > +ret = vhost_dev_get_inflight(&vsc->dev,
> > > +vs->conf.virtqueue_size,
> > > +vsc->inflight);
> > > +if (ret < 0) {
> > > +error_report("Error getting inflight: %d", -ret);
> > > +goto err_guest_notifiers;
> > > +}
> > > +}
> > > +
> > > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > +if (ret < 0) {
> > > +error_report("Error setting inflight: %d", -ret);
> > > +goto err_guest_notifiers;
> > > +}
> > > }
> > >
> > > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > return ret;
> > >
> > > err_guest_notifiers:
> > > -g_free(vsc->inflight);
> > > -vsc->inflight = NULL;
> > > -
> > > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > err_host_notifiers:
> > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > > }
> > > assert(ret >= 0);
> > >
> > > -if (vsc->inflight) {
> > > -vhost_dev_free_inflight(vsc->inflight);
> > > -g_free(vsc->inflight);
> > > -vsc->inflight = NULL;
> > > -}
> > > -
> > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > }
> > >
> > > --
> > > 2.41.0
> > >
> >

Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

[Raphael Norwitz]

> On Sep 22, 2023, at 7:46 AM, Li Feng  wrote:
> 
> Currently the get_inflight_fd will be sent every time the device is started, 
> and
> the backend will allocate shared memory to save the inflight state. If the
> backend finds that it receives the second get_inflight_fd, it will release the
> previous shared memory, which breaks inflight working logic.
> 
> This patch is a preparation for the following patches.

This looks identical to the v3 patch I reviewed? If I’ve missed something can 
you please point it out?


> Signed-off-by: Li Feng 
> ---
> hw/scsi/vhost-scsi-common.c | 37 ++---
> 1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a06f01af26..a61cd0e907 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> 
> vsc->dev.acked_features = vdev->guest_features;
> 
> -assert(vsc->inflight == NULL);
> -vsc->inflight = g_new0(struct vhost_inflight, 1);
> -ret = vhost_dev_get_inflight(&vsc->dev,
> - vs->conf.virtqueue_size,
> - vsc->inflight);
> +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> -error_report("Error get inflight: %d", -ret);
> +error_report("Error setting inflight format: %d", -ret);
> goto err_guest_notifiers;
> }
> 
> -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> -if (ret < 0) {
> -error_report("Error set inflight: %d", -ret);
> -goto err_guest_notifiers;
> +if (vsc->inflight) {
> +if (!vsc->inflight->addr) {
> +ret = vhost_dev_get_inflight(&vsc->dev,
> +vs->conf.virtqueue_size,
> +vsc->inflight);
> +if (ret < 0) {
> +error_report("Error getting inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> +}
> +
> +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> +if (ret < 0) {
> +error_report("Error setting inflight: %d", -ret);
> +goto err_guest_notifiers;
> +}
> }
> 
> ret = vhost_dev_start(&vsc->dev, vdev, true);
> @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> return ret;
> 
> err_guest_notifiers:
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -
> k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> err_host_notifiers:
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> }
> assert(ret >= 0);
> 
> -if (vsc->inflight) {
> -vhost_dev_free_inflight(vsc->inflight);
> -g_free(vsc->inflight);
> -vsc->inflight = NULL;
> -}
> -
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> }
> 
> -- 
> 2.41.0
> 

[PATCH v6 1/5] vhost-user-common: send get_inflight_fd once

[Li Feng]

Currently the get_inflight_fd will be sent every time the device is started, and
the backend will allocate shared memory to save the inflight state. If the
backend finds that it receives the second get_inflight_fd, it will release the
previous shared memory, which breaks inflight working logic.

This patch is a preparation for the following patches.

Signed-off-by: Li Feng 
---
 hw/scsi/vhost-scsi-common.c | 37 ++---
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a06f01af26..a61cd0e907 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
 
 vsc->dev.acked_features = vdev->guest_features;
 
-assert(vsc->inflight == NULL);
-vsc->inflight = g_new0(struct vhost_inflight, 1);
-ret = vhost_dev_get_inflight(&vsc->dev,
- vs->conf.virtqueue_size,
- vsc->inflight);
+ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
 if (ret < 0) {
-error_report("Error get inflight: %d", -ret);
+error_report("Error setting inflight format: %d", -ret);
 goto err_guest_notifiers;
 }
 
-ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
-if (ret < 0) {
-error_report("Error set inflight: %d", -ret);
-goto err_guest_notifiers;
+if (vsc->inflight) {
+if (!vsc->inflight->addr) {
+ret = vhost_dev_get_inflight(&vsc->dev,
+vs->conf.virtqueue_size,
+vsc->inflight);
+if (ret < 0) {
+error_report("Error getting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
+}
+
+ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
+if (ret < 0) {
+error_report("Error setting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
 }
 
 ret = vhost_dev_start(&vsc->dev, vdev, true);
@@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
 return ret;
 
 err_guest_notifiers:
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-
 k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
 err_host_notifiers:
 vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
 }
 assert(ret >= 0);
 
-if (vsc->inflight) {
-vhost_dev_free_inflight(vsc->inflight);
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-}
-
 vhost_dev_disable_notifiers(&vsc->dev, vdev);
 }
 
-- 
2.41.0