[PATCH V3 0/3] flush workers on suspend

[PATCH V3 0/3] flush workers on suspend

[Steve Sistare]

Flush to guarantee no workers are running when suspend returns,
for vdpa, vdpa_sim, and vduse.  (mlx5 already does so, via the path
mlx5_vdpa_suspend -> unregister_link_notifier -> flush_workqueue.)

Changes in V2:
  - renamed "vduse: suspend" (was vduse: flush workers on suspend)
  - call vhost_dev_flush unconditionally in "vhost-vdpa: flush workers on suspend"

Changes in v3:
  - rewrote vdpa_sim patch

Steve Sistare (3):
  vhost-vdpa: flush workers on suspend
  vduse: suspend
  vdpa_sim: flush workers on suspend

 drivers/vdpa/vdpa_sim/vdpa_sim.c   | 16 ++++++++++++++--
 drivers/vdpa/vdpa_sim/vdpa_sim.h   |  1 +
 drivers/vdpa/vdpa_user/vduse_dev.c | 24 ++++++++++++++++++++++++
 drivers/vhost/vdpa.c               |  3 +++
 4 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.39.3

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

[Steve Sistare]

Flush to guarantee no workers are running when suspend returns.

Fixes: f345a0143b4d ("vhost-vdpa: uAPI to suspend the device")
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
 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

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

[Jason Wang]

On Mon, May 20, 2024 at 11:21 PM Steve Sistare
<steven.sistare@oracle.com> 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 <steven.sistare@oracle.com>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  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
>

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

[Steven Sistare]

On 5/20/2024 10:28 PM, Jason Wang wrote:
> On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> <steven.sistare@oracle.com> 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 <steven.sistare@oracle.com>
>> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>>   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

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

[Jason Wang]

On Tue, May 21, 2024 at 9:39 PM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 5/20/2024 10:28 PM, Jason Wang wrote:
> > On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> > <steven.sistare@oracle.com> 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 <steven.sistare@oracle.com>
> >> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >>   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
>
>
>

[PATCH V3 2/3] vduse: suspend

[Steve Sistare]

Support the suspend operation.  There is little to do, except flush to
guarantee no workers are running when suspend returns.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 73c89701fc9d..7dc46f771f12 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -472,6 +472,18 @@ static void vduse_dev_reset(struct vduse_dev *dev)
 	up_write(&dev->rwsem);
 }
 
+static void vduse_flush_work(struct vduse_dev *dev)
+{
+	flush_work(&dev->inject);
+
+	for (int i = 0; i < dev->vq_num; i++) {
+		struct vduse_virtqueue *vq = dev->vqs[i];
+
+		flush_work(&vq->inject);
+		flush_work(&vq->kick);
+	}
+}
+
 static int vduse_vdpa_set_vq_address(struct vdpa_device *vdpa, u16 idx,
 				u64 desc_area, u64 driver_area,
 				u64 device_area)
@@ -724,6 +736,17 @@ static int vduse_vdpa_reset(struct vdpa_device *vdpa)
 	return ret;
 }
 
+static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
+{
+	struct vduse_dev *dev = vdpa_to_vduse(vdpa);
+
+	down_write(&dev->rwsem);
+	vduse_flush_work(dev);
+	up_write(&dev->rwsem);
+
+	return 0;
+}
+
 static u32 vduse_vdpa_get_generation(struct vdpa_device *vdpa)
 {
 	struct vduse_dev *dev = vdpa_to_vduse(vdpa);
@@ -806,6 +829,7 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
 	.set_vq_affinity	= vduse_vdpa_set_vq_affinity,
 	.get_vq_affinity	= vduse_vdpa_get_vq_affinity,
 	.reset			= vduse_vdpa_reset,
+	.suspend		= vduse_vdpa_suspend,
 	.set_map		= vduse_vdpa_set_map,
 	.free			= vduse_vdpa_free,
 };
-- 
2.39.3

Re: [PATCH V3 2/3] vduse: suspend

[Jason Wang]

On Mon, May 20, 2024 at 11:21 PM Steve Sistare
<steven.sistare@oracle.com> wrote:
>
> Support the suspend operation.  There is little to do, except flush to
> guarantee no workers are running when suspend returns.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 73c89701fc9d..7dc46f771f12 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -472,6 +472,18 @@ static void vduse_dev_reset(struct vduse_dev *dev)
>         up_write(&dev->rwsem);
>  }
>
> +static void vduse_flush_work(struct vduse_dev *dev)
> +{
> +       flush_work(&dev->inject);
> +
> +       for (int i = 0; i < dev->vq_num; i++) {
> +               struct vduse_virtqueue *vq = dev->vqs[i];
> +
> +               flush_work(&vq->inject);
> +               flush_work(&vq->kick);
> +       }
> +}
> +
>  static int vduse_vdpa_set_vq_address(struct vdpa_device *vdpa, u16 idx,
>                                 u64 desc_area, u64 driver_area,
>                                 u64 device_area)
> @@ -724,6 +736,17 @@ static int vduse_vdpa_reset(struct vdpa_device *vdpa)
>         return ret;
>  }
>
> +static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
> +{
> +       struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> +
> +       down_write(&dev->rwsem);
> +       vduse_flush_work(dev);
> +       up_write(&dev->rwsem);

Can this forbid the new work to be scheduled?

static int vduse_dev_queue_irq_work(struct vduse_dev *dev,
                                    struct work_struct *irq_work,
                                    int irq_effective_cpu)
{
        int ret = -EINVAL;

        down_read(&dev->rwsem);
        if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
                goto unlock;

        ret = 0;
        if (irq_effective_cpu == IRQ_UNBOUND)
                queue_work(vduse_irq_wq, irq_work);
        else
                queue_work_on(irq_effective_cpu,
                      vduse_irq_bound_wq, irq_work);
unlock:
        up_read(&dev->rwsem);

        return ret;
}

Thanks

> +
> +       return 0;
> +}
> +
>  static u32 vduse_vdpa_get_generation(struct vdpa_device *vdpa)
>  {
>         struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> @@ -806,6 +829,7 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
>         .set_vq_affinity        = vduse_vdpa_set_vq_affinity,
>         .get_vq_affinity        = vduse_vdpa_get_vq_affinity,
>         .reset                  = vduse_vdpa_reset,
> +       .suspend                = vduse_vdpa_suspend,
>         .set_map                = vduse_vdpa_set_map,
>         .free                   = vduse_vdpa_free,
>  };
> --
> 2.39.3
>

Re: [PATCH V3 2/3] vduse: suspend

[Steven Sistare]

On 5/20/2024 10:30 PM, Jason Wang wrote:
> On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> <steven.sistare@oracle.com> wrote:
>>
>> Support the suspend operation.  There is little to do, except flush to
>> guarantee no workers are running when suspend returns.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   drivers/vdpa/vdpa_user/vduse_dev.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 73c89701fc9d..7dc46f771f12 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -472,6 +472,18 @@ static void vduse_dev_reset(struct vduse_dev *dev)
>>          up_write(&dev->rwsem);
>>   }
>>
>> +static void vduse_flush_work(struct vduse_dev *dev)
>> +{
>> +       flush_work(&dev->inject);
>> +
>> +       for (int i = 0; i < dev->vq_num; i++) {
>> +               struct vduse_virtqueue *vq = dev->vqs[i];
>> +
>> +               flush_work(&vq->inject);
>> +               flush_work(&vq->kick);
>> +       }
>> +}
>> +
>>   static int vduse_vdpa_set_vq_address(struct vdpa_device *vdpa, u16 idx,
>>                                  u64 desc_area, u64 driver_area,
>>                                  u64 device_area)
>> @@ -724,6 +736,17 @@ static int vduse_vdpa_reset(struct vdpa_device *vdpa)
>>          return ret;
>>   }
>>
>> +static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
>> +{
>> +       struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>> +
>> +       down_write(&dev->rwsem);
>> +       vduse_flush_work(dev);
>> +       up_write(&dev->rwsem);
> 
> Can this forbid the new work to be scheduled?

Are you suggesting I return an error below if the dev is suspended?
I can do that.

However, I now suspect this implementation of vduse_vdpa_suspend is not
complete in other ways, so I withdraw this patch pending future work.
Thanks for looking at it.

- Steve

> static int vduse_dev_queue_irq_work(struct vduse_dev *dev,
>                                      struct work_struct *irq_work,
>                                      int irq_effective_cpu)
> {
>          int ret = -EINVAL;
> 
>          down_read(&dev->rwsem);
>          if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
>                  goto unlock;
> 
>          ret = 0;
>          if (irq_effective_cpu == IRQ_UNBOUND)
>                  queue_work(vduse_irq_wq, irq_work);
>          else
>                  queue_work_on(irq_effective_cpu,
>                        vduse_irq_bound_wq, irq_work);
> unlock:
>          up_read(&dev->rwsem);
> 
>          return ret;
> }
> 
> Thanks
> 
>> +
>> +       return 0;
>> +}
>> +
>>   static u32 vduse_vdpa_get_generation(struct vdpa_device *vdpa)
>>   {
>>          struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>> @@ -806,6 +829,7 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
>>          .set_vq_affinity        = vduse_vdpa_set_vq_affinity,
>>          .get_vq_affinity        = vduse_vdpa_get_vq_affinity,
>>          .reset                  = vduse_vdpa_reset,
>> +       .suspend                = vduse_vdpa_suspend,
>>          .set_map                = vduse_vdpa_set_map,
>>          .free                   = vduse_vdpa_free,
>>   };
>> --
>> 2.39.3
>>
> 

Re: [PATCH V3 2/3] vduse: suspend

[Jason Wang]

On Tue, May 21, 2024 at 9:39 PM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 5/20/2024 10:30 PM, Jason Wang wrote:
> > On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> > <steven.sistare@oracle.com> wrote:
> >>
> >> Support the suspend operation.  There is little to do, except flush to
> >> guarantee no workers are running when suspend returns.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>   drivers/vdpa/vdpa_user/vduse_dev.c | 24 ++++++++++++++++++++++++
> >>   1 file changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> index 73c89701fc9d..7dc46f771f12 100644
> >> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> @@ -472,6 +472,18 @@ static void vduse_dev_reset(struct vduse_dev *dev)
> >>          up_write(&dev->rwsem);
> >>   }
> >>
> >> +static void vduse_flush_work(struct vduse_dev *dev)
> >> +{
> >> +       flush_work(&dev->inject);
> >> +
> >> +       for (int i = 0; i < dev->vq_num; i++) {
> >> +               struct vduse_virtqueue *vq = dev->vqs[i];
> >> +
> >> +               flush_work(&vq->inject);
> >> +               flush_work(&vq->kick);
> >> +       }
> >> +}
> >> +
> >>   static int vduse_vdpa_set_vq_address(struct vdpa_device *vdpa, u16 idx,
> >>                                  u64 desc_area, u64 driver_area,
> >>                                  u64 device_area)
> >> @@ -724,6 +736,17 @@ static int vduse_vdpa_reset(struct vdpa_device *vdpa)
> >>          return ret;
> >>   }
> >>
> >> +static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
> >> +{
> >> +       struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> >> +
> >> +       down_write(&dev->rwsem);
> >> +       vduse_flush_work(dev);
> >> +       up_write(&dev->rwsem);
> >
> > Can this forbid the new work to be scheduled?
>
> Are you suggesting I return an error below if the dev is suspended?
> I can do that.

I mean the irq injection work can still be scheduled after vduse_vdpa_suspend().

>
> However, I now suspect this implementation of vduse_vdpa_suspend is not
> complete in other ways, so I withdraw this patch pending future work.
> Thanks for looking at it.

Ok.

Thanks

>
> - Steve
>
> > static int vduse_dev_queue_irq_work(struct vduse_dev *dev,
> >                                      struct work_struct *irq_work,
> >                                      int irq_effective_cpu)
> > {
> >          int ret = -EINVAL;
> >
> >          down_read(&dev->rwsem);
> >          if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> >                  goto unlock;
> >
> >          ret = 0;
> >          if (irq_effective_cpu == IRQ_UNBOUND)
> >                  queue_work(vduse_irq_wq, irq_work);
> >          else
> >                  queue_work_on(irq_effective_cpu,
> >                        vduse_irq_bound_wq, irq_work);
> > unlock:
> >          up_read(&dev->rwsem);
> >
> >          return ret;
> > }
> >
> > Thanks
> >
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static u32 vduse_vdpa_get_generation(struct vdpa_device *vdpa)
> >>   {
> >>          struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> >> @@ -806,6 +829,7 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> >>          .set_vq_affinity        = vduse_vdpa_set_vq_affinity,
> >>          .get_vq_affinity        = vduse_vdpa_get_vq_affinity,
> >>          .reset                  = vduse_vdpa_reset,
> >> +       .suspend                = vduse_vdpa_suspend,
> >>          .set_map                = vduse_vdpa_set_map,
> >>          .free                   = vduse_vdpa_free,
> >>   };
> >> --
> >> 2.39.3
> >>
> >
>

[PATCH V3 3/3] vdpa_sim: flush workers on suspend

[Steve Sistare]

Flush to guarantee no workers are running when suspend returns.
Add a lock to enforce ordering between clearing running, flushing,
and posting new work in vdpasim_kick_vq.  It must be a spin lock
because vdpasim_kick_vq may be reached va eventfd_write.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 16 ++++++++++++++--
 drivers/vdpa/vdpa_sim/vdpa_sim.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 8ffea8430f95..67ed49d95bf0 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -322,7 +322,7 @@ static u16 vdpasim_get_vq_size(struct vdpa_device *vdpa, u16 idx)
 		return VDPASIM_QUEUE_MAX;
 }
 
-static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
+static void vdpasim_do_kick_vq(struct vdpa_device *vdpa, u16 idx)
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 	struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
@@ -337,6 +337,15 @@ static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
 		vdpasim_schedule_work(vdpasim);
 }
 
+static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+	spin_lock(&vdpasim->kick_lock);
+	vdpasim_do_kick_vq(vdpa, idx);
+	spin_unlock(&vdpasim->kick_lock);
+}
+
 static void vdpasim_set_vq_cb(struct vdpa_device *vdpa, u16 idx,
 			      struct vdpa_callback *cb)
 {
@@ -520,8 +529,11 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 
 	mutex_lock(&vdpasim->mutex);
+	spin_lock(&vdpasim->kick_lock);
 	vdpasim->running = false;
+	spin_unlock(&vdpasim->kick_lock);
 	mutex_unlock(&vdpasim->mutex);
+	kthread_flush_work(&vdpasim->work);
 
 	return 0;
 }
@@ -537,7 +549,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
 	if (vdpasim->pending_kick) {
 		/* Process pending descriptors */
 		for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
-			vdpasim_kick_vq(vdpa, i);
+			vdpasim_do_kick_vq(vdpa, i);
 
 		vdpasim->pending_kick = false;
 	}
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index bb137e479763..5eb6ca9c5ec5 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -75,6 +75,7 @@ struct vdpasim {
 	bool pending_kick;
 	/* spinlock to synchronize iommu table */
 	spinlock_t iommu_lock;
+	spinlock_t kick_lock;
 };
 
 struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr,
-- 
2.39.3

Re: [PATCH V3 3/3] vdpa_sim: flush workers on suspend

[Jason Wang]

On Mon, May 20, 2024 at 11:21 PM Steve Sistare
<steven.sistare@oracle.com> wrote:
>
> Flush to guarantee no workers are running when suspend returns.
> Add a lock to enforce ordering between clearing running, flushing,
> and posting new work in vdpasim_kick_vq.  It must be a spin lock
> because vdpasim_kick_vq may be reached va eventfd_write.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 16 ++++++++++++++--
>  drivers/vdpa/vdpa_sim/vdpa_sim.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 8ffea8430f95..67ed49d95bf0 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -322,7 +322,7 @@ static u16 vdpasim_get_vq_size(struct vdpa_device *vdpa, u16 idx)
>                 return VDPASIM_QUEUE_MAX;
>  }
>
> -static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
> +static void vdpasim_do_kick_vq(struct vdpa_device *vdpa, u16 idx)
>  {
>         struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>         struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> @@ -337,6 +337,15 @@ static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
>                 vdpasim_schedule_work(vdpasim);
>  }
>
> +static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
> +{
> +       struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> +
> +       spin_lock(&vdpasim->kick_lock);
> +       vdpasim_do_kick_vq(vdpa, idx);
> +       spin_unlock(&vdpasim->kick_lock);
> +}
> +
>  static void vdpasim_set_vq_cb(struct vdpa_device *vdpa, u16 idx,
>                               struct vdpa_callback *cb)
>  {
> @@ -520,8 +529,11 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
>         struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>
>         mutex_lock(&vdpasim->mutex);
> +       spin_lock(&vdpasim->kick_lock);
>         vdpasim->running = false;
> +       spin_unlock(&vdpasim->kick_lock);
>         mutex_unlock(&vdpasim->mutex);
> +       kthread_flush_work(&vdpasim->work);
>
>         return 0;
>  }
> @@ -537,7 +549,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
>         if (vdpasim->pending_kick) {
>                 /* Process pending descriptors */
>                 for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
> -                       vdpasim_kick_vq(vdpa, i);
> +                       vdpasim_do_kick_vq(vdpa, i);
>
>                 vdpasim->pending_kick = false;
>         }
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index bb137e479763..5eb6ca9c5ec5 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -75,6 +75,7 @@ struct vdpasim {
>         bool pending_kick;
>         /* spinlock to synchronize iommu table */
>         spinlock_t iommu_lock;
> +       spinlock_t kick_lock;

It looks to me this is not initialized?

Thanks

>  };
>
>  struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr,
> --
> 2.39.3
>

Re: [PATCH V3 3/3] vdpa_sim: flush workers on suspend

[Steven Sistare]

On 5/20/2024 10:32 PM, Jason Wang wrote:
> On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> <steven.sistare@oracle.com> wrote:
>>
>> Flush to guarantee no workers are running when suspend returns.
>> Add a lock to enforce ordering between clearing running, flushing,
>> and posting new work in vdpasim_kick_vq.  It must be a spin lock
>> because vdpasim_kick_vq may be reached va eventfd_write.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   drivers/vdpa/vdpa_sim/vdpa_sim.c | 16 ++++++++++++++--
>>   drivers/vdpa/vdpa_sim/vdpa_sim.h |  1 +
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> index 8ffea8430f95..67ed49d95bf0 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> @@ -322,7 +322,7 @@ static u16 vdpasim_get_vq_size(struct vdpa_device *vdpa, u16 idx)
>>                  return VDPASIM_QUEUE_MAX;
>>   }
>>
>> -static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
>> +static void vdpasim_do_kick_vq(struct vdpa_device *vdpa, u16 idx)
>>   {
>>          struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>          struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
>> @@ -337,6 +337,15 @@ static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
>>                  vdpasim_schedule_work(vdpasim);
>>   }
>>
>> +static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
>> +{
>> +       struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>> +
>> +       spin_lock(&vdpasim->kick_lock);
>> +       vdpasim_do_kick_vq(vdpa, idx);
>> +       spin_unlock(&vdpasim->kick_lock);
>> +}
>> +
>>   static void vdpasim_set_vq_cb(struct vdpa_device *vdpa, u16 idx,
>>                                struct vdpa_callback *cb)
>>   {
>> @@ -520,8 +529,11 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
>>          struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>
>>          mutex_lock(&vdpasim->mutex);
>> +       spin_lock(&vdpasim->kick_lock);
>>          vdpasim->running = false;
>> +       spin_unlock(&vdpasim->kick_lock);
>>          mutex_unlock(&vdpasim->mutex);
>> +       kthread_flush_work(&vdpasim->work);
>>
>>          return 0;
>>   }
>> @@ -537,7 +549,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
>>          if (vdpasim->pending_kick) {
>>                  /* Process pending descriptors */
>>                  for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
>> -                       vdpasim_kick_vq(vdpa, i);
>> +                       vdpasim_do_kick_vq(vdpa, i);
>>
>>                  vdpasim->pending_kick = false;
>>          }
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> index bb137e479763..5eb6ca9c5ec5 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> @@ -75,6 +75,7 @@ struct vdpasim {
>>          bool pending_kick;
>>          /* spinlock to synchronize iommu table */
>>          spinlock_t iommu_lock;
>> +       spinlock_t kick_lock;
> 
> It looks to me this is not initialized?

Yup, I lost that line while fiddling with different locking schemes.
Thanks, will fix in V4.

@@ -236,6 +236,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr 
*dev_attr,

         mutex_init(&vdpasim->mutex);
         spin_lock_init(&vdpasim->iommu_lock);
+       spin_lock_init(&vdpasim->kick_lock);

With that fix, does this patch earn your RB?

- Steve

>>   };
>>
>>   struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr,
>> --
>> 2.39.3
>>
> 

Re: [PATCH V3 3/3] vdpa_sim: flush workers on suspend

[Jason Wang]

On Tue, May 21, 2024 at 9:39 PM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 5/20/2024 10:32 PM, Jason Wang wrote:
> > On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> > <steven.sistare@oracle.com> wrote:
> >>
> >> Flush to guarantee no workers are running when suspend returns.
> >> Add a lock to enforce ordering between clearing running, flushing,
> >> and posting new work in vdpasim_kick_vq.  It must be a spin lock
> >> because vdpasim_kick_vq may be reached va eventfd_write.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>   drivers/vdpa/vdpa_sim/vdpa_sim.c | 16 ++++++++++++++--
> >>   drivers/vdpa/vdpa_sim/vdpa_sim.h |  1 +
> >>   2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> index 8ffea8430f95..67ed49d95bf0 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> @@ -322,7 +322,7 @@ static u16 vdpasim_get_vq_size(struct vdpa_device *vdpa, u16 idx)
> >>                  return VDPASIM_QUEUE_MAX;
> >>   }
> >>
> >> -static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
> >> +static void vdpasim_do_kick_vq(struct vdpa_device *vdpa, u16 idx)
> >>   {
> >>          struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> >>          struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >> @@ -337,6 +337,15 @@ static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
> >>                  vdpasim_schedule_work(vdpasim);
> >>   }
> >>
> >> +static void vdpasim_kick_vq(struct vdpa_device *vdpa, u16 idx)
> >> +{
> >> +       struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> >> +
> >> +       spin_lock(&vdpasim->kick_lock);
> >> +       vdpasim_do_kick_vq(vdpa, idx);
> >> +       spin_unlock(&vdpasim->kick_lock);
> >> +}
> >> +
> >>   static void vdpasim_set_vq_cb(struct vdpa_device *vdpa, u16 idx,
> >>                                struct vdpa_callback *cb)
> >>   {
> >> @@ -520,8 +529,11 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
> >>          struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> >>
> >>          mutex_lock(&vdpasim->mutex);
> >> +       spin_lock(&vdpasim->kick_lock);
> >>          vdpasim->running = false;
> >> +       spin_unlock(&vdpasim->kick_lock);
> >>          mutex_unlock(&vdpasim->mutex);
> >> +       kthread_flush_work(&vdpasim->work);
> >>
> >>          return 0;
> >>   }
> >> @@ -537,7 +549,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
> >>          if (vdpasim->pending_kick) {
> >>                  /* Process pending descriptors */
> >>                  for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
> >> -                       vdpasim_kick_vq(vdpa, i);
> >> +                       vdpasim_do_kick_vq(vdpa, i);
> >>
> >>                  vdpasim->pending_kick = false;
> >>          }
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >> index bb137e479763..5eb6ca9c5ec5 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >> @@ -75,6 +75,7 @@ struct vdpasim {
> >>          bool pending_kick;
> >>          /* spinlock to synchronize iommu table */
> >>          spinlock_t iommu_lock;
> >> +       spinlock_t kick_lock;
> >
> > It looks to me this is not initialized?
>
> Yup, I lost that line while fiddling with different locking schemes.
> Thanks, will fix in V4.
>
> @@ -236,6 +236,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
> *dev_attr,
>
>          mutex_init(&vdpasim->mutex);
>          spin_lock_init(&vdpasim->iommu_lock);
> +       spin_lock_init(&vdpasim->kick_lock);
>
> With that fix, does this patch earn your RB?

Yes.

Thanks

>
> - Steve
>
> >>   };
> >>
> >>   struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr,
> >> --
> >> 2.39.3
> >>
> >
>