2022-05-19 17:49:43

by Eugenio Perez Martin

Subject: [PATCH] vdpasim: allow to enable a vq repeatedly

Code must be resilient to enable a queue many times.

At the moment the queue is resetting so it's definitely not the expected
behavior.

Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
Cc: [email protected]
Signed-off-by: Eugenio Pérez <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index ddbe142af09a..b53cd00ad161 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -355,9 +355,10 @@ static void vdpasim_set_vq_ready(struct vdpa_device *vdpa, u16 idx, bool ready)
struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];

spin_lock(&vdpasim->lock);
- vq->ready = ready;
- if (vq->ready)
+ if (!vq->ready) {
+ vq->ready = ready;
vdpasim_queue_ready(vdpasim, idx);
+ }
spin_unlock(&vdpasim->lock);
}

--
2.27.0

2022-05-20 08:12:22

by Stefano Garzarella

Subject: Re: [PATCH] vdpasim: allow to enable a vq repeatedly

On Thu, May 19, 2022 at 04:31:45PM +0200, Eugenio P?rez wrote:
>Code must be resilient to enable a queue many times.
>
>At the moment the queue is resetting so it's definitely not the expected
>behavior.
>
>Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
>Cc: [email protected]
>Signed-off-by: Eugenio P?rez <[email protected]>
>---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index ddbe142af09a..b53cd00ad161 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>@@ -355,9 +355,10 @@ static void vdpasim_set_vq_ready(struct vdpa_device *vdpa, u16 idx, bool ready)
> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
>
> spin_lock(&vdpasim->lock);
>- vq->ready = ready;
>- if (vq->ready)
>+ if (!vq->ready) {
>+ vq->ready = ready;
> vdpasim_queue_ready(vdpasim, idx);
>+ }

But this way the first time vq->ready is set to true, then it will never
be set back to false.

Should we leave the assignment out of the block?
Maybe after the if block to avoid the problem we are fixing.

Thanks,
Stefano

2022-05-20 15:25:03

by Eugenio Perez Martin

Subject: Re: [PATCH] vdpasim: allow to enable a vq repeatedly

On Thu, May 19, 2022 at 4:48 PM Stefano Garzarella <[email protected]> wrote:
>
> On Thu, May 19, 2022 at 04:31:45PM +0200, Eugenio Pérez wrote:
> >Code must be resilient to enable a queue many times.
> >
> >At the moment the queue is resetting so it's definitely not the expected
> >behavior.
> >
> >Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> >Cc: [email protected]
> >Signed-off-by: Eugenio Pérez <[email protected]>
> >---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >index ddbe142af09a..b53cd00ad161 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >@@ -355,9 +355,10 @@ static void vdpasim_set_vq_ready(struct vdpa_device *vdpa, u16 idx, bool ready)
> > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >
> > spin_lock(&vdpasim->lock);
> >- vq->ready = ready;
> >- if (vq->ready)
> >+ if (!vq->ready) {
> >+ vq->ready = ready;
> > vdpasim_queue_ready(vdpasim, idx);
> >+ }
>
> But this way the first time vq->ready is set to true, then it will never
> be set back to false.
>

You're right, I had in mind to reset the flow before enabling as the
only possibility.

Sending v2 with that part fixed, thanks!

> Should we leave the assignment out of the block?
> Maybe after the if block to avoid the problem we are fixing.
>
> Thanks,
> Stefano
>