2024-05-20 15:21:33

by Steven Sistare

Subject: [PATCH V3 1/3] vhost-vdpa: flush workers on suspend

Flush to guarantee no workers are running when suspend returns.

Fixes: f345a0143b4d ("vhost-vdpa: uAPI to suspend the device")
Signed-off-by: Steve Sistare <[email protected]>
Acked-by: Eugenio Pérez <[email protected]>
---
drivers/vhost/vdpa.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ba52d128aeb7..189596caaec9 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -594,6 +594,7 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
struct vdpa_device *vdpa = v->vdpa;
const struct vdpa_config_ops *ops = vdpa->config;
int ret;
+ struct vhost_dev *vdev = &v->vdev;

if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
return 0;
@@ -601,6 +602,8 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
if (!ops->suspend)
return -EOPNOTSUPP;

+ vhost_dev_flush(vdev);
+
ret = ops->suspend(vdpa);
if (!ret)
v->suspended = true;
--
2.39.3

2024-05-21 02:29:23

by Jason Wang

Subject: Re: [PATCH V3 1/3] vhost-vdpa: flush workers on suspend

On Mon, May 20, 2024 at 11:21 PM Steve Sistare
<[email protected]> wrote:
>
> Flush to guarantee no workers are running when suspend returns.
>
> Fixes: f345a0143b4d ("vhost-vdpa: uAPI to suspend the device")
> Signed-off-by: Steve Sistare <[email protected]>
> Acked-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vhost/vdpa.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ba52d128aeb7..189596caaec9 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -594,6 +594,7 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> struct vdpa_device *vdpa = v->vdpa;
> const struct vdpa_config_ops *ops = vdpa->config;
> int ret;
> + struct vhost_dev *vdev = &v->vdev;
>
> if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
> return 0;
> @@ -601,6 +602,8 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> if (!ops->suspend)
> return -EOPNOTSUPP;
>
> + vhost_dev_flush(vdev);

vhost-vDPA doesn't use workers, see:

vhost_dev_init(dev, vqs, nvqs, 0, 0, 0, false,
vhost_vdpa_process_iotlb_msg);

So I wonder if this is a must.

Thanks

> +
> ret = ops->suspend(vdpa);
> if (!ret)
> v->suspended = true;
> --
> 2.39.3
>

2024-05-21 13:39:04

by Steven Sistare

Subject: Re: [PATCH V3 1/3] vhost-vdpa: flush workers on suspend

On 5/20/2024 10:28 PM, Jason Wang wrote:
> On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> <[email protected]> wrote:
>>
>> Flush to guarantee no workers are running when suspend returns.
>>
>> Fixes: f345a0143b4d ("vhost-vdpa: uAPI to suspend the device")
>> Signed-off-by: Steve Sistare <[email protected]>
>> Acked-by: Eugenio Pérez <[email protected]>
>> ---
>> drivers/vhost/vdpa.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>> index ba52d128aeb7..189596caaec9 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -594,6 +594,7 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
>> struct vdpa_device *vdpa = v->vdpa;
>> const struct vdpa_config_ops *ops = vdpa->config;
>> int ret;
>> + struct vhost_dev *vdev = &v->vdev;
>>
>> if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
>> return 0;
>> @@ -601,6 +602,8 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
>> if (!ops->suspend)
>> return -EOPNOTSUPP;
>>
>> + vhost_dev_flush(vdev);
>
> vhost-vDPA doesn't use workers, see:
>
> vhost_dev_init(dev, vqs, nvqs, 0, 0, 0, false,
> vhost_vdpa_process_iotlb_msg);
>
> So I wonder if this is a must.

True, but I am adding this to be future proof. I could instead log a warning
or an error message if vhost_vdpa_suspend is called and v->vdev.use_worker=true,
but IMO we should just fix it, given that the fix is trivial.

- Steve

2024-05-22 03:41:53

by Jason Wang

Subject: Re: [PATCH V3 1/3] vhost-vdpa: flush workers on suspend

On Tue, May 21, 2024 at 9:39 PM Steven Sistare
<[email protected]> wrote:
>
> On 5/20/2024 10:28 PM, Jason Wang wrote:
> > On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> > <[email protected]> wrote:
> >>
> >> Flush to guarantee no workers are running when suspend returns.
> >>
> >> Fixes: f345a0143b4d ("vhost-vdpa: uAPI to suspend the device")
> >> Signed-off-by: Steve Sistare <[email protected]>
> >> Acked-by: Eugenio Pérez <[email protected]>
> >> ---
> >> drivers/vhost/vdpa.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> >> index ba52d128aeb7..189596caaec9 100644
> >> --- a/drivers/vhost/vdpa.c
> >> +++ b/drivers/vhost/vdpa.c
> >> @@ -594,6 +594,7 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> >> struct vdpa_device *vdpa = v->vdpa;
> >> const struct vdpa_config_ops *ops = vdpa->config;
> >> int ret;
> >> + struct vhost_dev *vdev = &v->vdev;
> >>
> >> if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
> >> return 0;
> >> @@ -601,6 +602,8 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> >> if (!ops->suspend)
> >> return -EOPNOTSUPP;
> >>
> >> + vhost_dev_flush(vdev);
> >
> > vhost-vDPA doesn't use workers, see:
> >
> > vhost_dev_init(dev, vqs, nvqs, 0, 0, 0, false,
> > vhost_vdpa_process_iotlb_msg);
> >
> > So I wonder if this is a must.
>
> True, but I am adding this to be future proof. I could instead log a warning
> or an error message if vhost_vdpa_suspend is called and v->vdev.use_worker=true,
> but IMO we should just fix it, given that the fix is trivial.

I meant we need to know if it fixes any actual issue or not.

Thanks

>
> - Steve
>
>
>