[PATCH] vhost: clean up outstanding buffers before setting vring

The outstanding DMA buffers need to be clean up before setting vring in
vhost. Otherwise the vring would be out of sync.

Signed-off-by: Shirley Ma<redacted>
---

 drivers/vhost/vhost.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c14c42b..d6315b4 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -445,8 +445,10 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
 			vhost_poll_flush(&dev->vqs[i].poll);
 		}
 		/* Wait for all lower device DMAs done. */
-		if (dev->vqs[i].ubufs)
+		if (dev->vqs[i].ubufs) {
 			vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
+			kfree(dev->vqs[i].ubufs);
+		}
 
 		/* Signal guest as appropriate. */
 		vhost_zerocopy_signal_used(&dev->vqs[i]);

@@ -651,6 +653,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 	vq = d->vqs + idx;
 
 	mutex_lock(&vq->mutex);
+	/* Wait for all lower device DMAs done. */
+	if (vq->ubufs)
+		vhost_ubuf_put_and_wait(vq->ubufs);
+
+	/* Signal guest as appropriate. */
+	vhost_zerocopy_signal_used(vq);
 
 	switch (ioctl) {
 	case VHOST_SET_VRING_NUM:

@@ -1592,7 +1600,6 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
 {
 	kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
 	wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
-	kfree(ubufs);
 }
 
 void vhost_zerocopy_callback(void *arg)

On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:

The outstanding DMA buffers need to be clean up before setting vring in
vhost. Otherwise the vring would be out of sync.

Signed-off-by: Shirley Ma<redacted>

I suspect what is missing is calling
vhost_zerocopy_signal_used then?

If yes we probably should do it after
changing the backend, not on vring set.

Could you elaborate on the problem you observe please?
At least in theory, existing code flushes outstanding
requests when backend is changed.
And since vring set verifies no backend is active,
we should be fine?

Won't this leak memory when ubufs are switched in vhost_net_set_backend?

 }
 
 void vhost_zerocopy_callback(void *arg)

On Tue, 2011-07-19 at 22:49 +0300, Michael S. Tsirkin wrote:

On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:

quoted

The outstanding DMA buffers need to be clean up before setting vring

in

quoted

vhost. Otherwise the vring would be out of sync.

Signed-off-by: Shirley Ma<redacted>

I suspect what is missing is calling
vhost_zerocopy_signal_used then?

If yes we probably should do it after
changing the backend, not on vring set.

I think vhost_zerocopy_signal_used might not be sufficient. But we can
test it out by remove/reloading the guest virtio_net module.

quoted

---

 drivers/vhost/vhost.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c14c42b..d6315b4 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -445,8 +445,10 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
                      vhost_poll_flush(&dev->vqs[i].poll);
              }
              /* Wait for all lower device DMAs done. */
-             if (dev->vqs[i].ubufs)
+             if (dev->vqs[i].ubufs) {
                      vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
+                     kfree(dev->vqs[i].ubufs);
+             }
 
              /* Signal guest as appropriate. */
              vhost_zerocopy_signal_used(&dev->vqs[i]);

@@ -651,6 +653,12 @@ static long vhost_set_vring(struct vhost_dev

*d, int ioctl, void __user *argp)

quoted

      vq = d->vqs + idx;
 
      mutex_lock(&vq->mutex);
+     /* Wait for all lower device DMAs done. */
+     if (vq->ubufs)
+             vhost_ubuf_put_and_wait(vq->ubufs);

Could you elaborate on the problem you observe please?
At least in theory, existing code flushes outstanding
requests when backend is changed.
And since vring set verifies no backend is active,
we should be fine?

The problem encounters when guest rmmod virtio_net module, then reload
the module, and configure the interface, it complains about some ring id
is not a head. With this patch, the problem is solved. 

quoted

+
+     /* Signal guest as appropriate. */
+     vhost_zerocopy_signal_used(vq);
 
      switch (ioctl) {
      case VHOST_SET_VRING_NUM:

@@ -1592,7 +1600,6 @@ void vhost_ubuf_put_and_wait(struct

vhost_ubuf_ref *ubufs)

quoted

 {
      kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
      wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
-     kfree(ubufs);

Won't this leak memory when ubufs are switched in
vhost_net_set_backend? 

Right, I forgot to check net.c, whenever it calls
vhot_ubuf_put_and_wait, it should call kfree(ubufs).

On Tue, Jul 19, 2011 at 01:50:53PM -0700, Shirley Ma wrote:

On Tue, 2011-07-19 at 22:49 +0300, Michael S. Tsirkin wrote:

quoted

On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:

quoted

The outstanding DMA buffers need to be clean up before setting vring

in

quoted

vhost. Otherwise the vring would be out of sync.

Signed-off-by: Shirley Ma<redacted>

I suspect what is missing is calling
vhost_zerocopy_signal_used then?

If yes we probably should do it after
changing the backend, not on vring set.

I think vhost_zerocopy_signal_used might not be sufficient.

If not, I'd like to understand what the root cause of
the problem is.

But we can
test it out by remove/reloading the guest virtio_net module.

Well, try out something like the below patch then.

The problem encounters when guest rmmod virtio_net module, then reload
the module, and configure the interface, it complains about some ring id
is not a head.

OK, good, such a problem decription belongs in the patch commit log.

With this patch, the problem is solved. 

Additional info you want to put in the commit log is what in the code
triggers the problem and how your patch fixes it.

quoted

quoted

+
+     /* Signal guest as appropriate. */
+     vhost_zerocopy_signal_used(vq);
 
      switch (ioctl) {
      case VHOST_SET_VRING_NUM:

@@ -1592,7 +1600,6 @@ void vhost_ubuf_put_and_wait(struct

vhost_ubuf_ref *ubufs)

quoted

 {
      kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
      wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
-     kfree(ubufs);

Won't this leak memory when ubufs are switched in
vhost_net_set_backend? 

Right, I forgot to check net.c, whenever it calls
vhot_ubuf_put_and_wait, it should call kfree(ubufs).

-->

vhost-net: update used ring on backend change

On backend change, we flushed out outstanding skbs
but forgot to update the used ring. Do that to
avoid losing heads.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 70ac604..248b250 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c

@@ -711,8 +711,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 
 	mutex_unlock(&vq->mutex);
 
-	if (oldubufs)
+	if (oldubufs) {
 		vhost_ubuf_put_and_wait(oldubufs);
+		mutex_lock(&vq->mutex);
+		vhost_zerocopy_signal_used(vq);
+		mutex_unlock(&vq->mutex);
+	}
 
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);

-- 
MST

On Wed, 2011-07-20 at 13:41 +0300, Michael S. Tsirkin wrote:

On Tue, Jul 19, 2011 at 01:50:53PM -0700, Shirley Ma wrote:

quoted

On Tue, 2011-07-19 at 22:49 +0300, Michael S. Tsirkin wrote:

quoted

On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:

quoted

The outstanding DMA buffers need to be clean up before setting

vring

quoted

quoted

in

quoted

vhost. Otherwise the vring would be out of sync.

Signed-off-by: Shirley Ma<redacted>

I suspect what is missing is calling
vhost_zerocopy_signal_used then?

If yes we probably should do it after
changing the backend, not on vring set.

I think vhost_zerocopy_signal_used might not be sufficient.

If not, I'd like to understand what the root cause of
the problem is.

quoted

But we can
test it out by remove/reloading the guest virtio_net module.

Well, try out something like the below patch then.

From the test results, below patch solves the problem. You can check in

this.

quoted

The problem encounters when guest rmmod virtio_net module, then

reload

quoted

the module, and configure the interface, it complains about some

ring id

quoted

is not a head.

OK, good, such a problem decription belongs in the patch commit log.

You can add this description in the change log. When removing and
reloading KVM guest virtio_net module, it complains vring data is NULL.
The vring is out of sync between vhost and virtio_net.