2024-05-20 15:21:50

by Steven Sistare

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

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 <[email protected]>
---
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

2024-05-21 02:31:27

by Jason Wang

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

On Mon, May 20, 2024 at 11:21 PM Steve Sistare
<[email protected]> 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 <[email protected]>
> ---
> 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
>

2024-05-21 13:39:50

by Steven Sistare

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

On 5/20/2024 10:30 PM, Jason Wang wrote:
> On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
>

2024-05-22 03:41:16

by Jason Wang

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

On Tue, May 21, 2024 at 9:39 PM Steven Sistare
<[email protected]> wrote:
>
> On 5/20/2024 10:30 PM, Jason Wang wrote:
> > On Mon, May 20, 2024 at 11:21 PM Steve Sistare
> > <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
> >>
> >
>