* [PATCH] vhost: clean up outstanding buffers before setting vring @ 2011-07-19 18:02 Shirley Ma 2011-07-19 19:49 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Shirley Ma @ 2011-07-19 18:02 UTC (permalink / raw) To: David Miller, mst; +Cc: netdev, jasowang 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<xma@us.ibm.com> --- 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) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring 2011-07-19 18:02 [PATCH] vhost: clean up outstanding buffers before setting vring Shirley Ma @ 2011-07-19 19:49 ` Michael S. Tsirkin 2011-07-19 20:50 ` Shirley Ma 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2011-07-19 19:49 UTC (permalink / raw) To: Shirley Ma; +Cc: David Miller, netdev, jasowang 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<xma@us.ibm.com> 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. > --- > > 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); 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? > + > + /* 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); Won't this leak memory when ubufs are switched in vhost_net_set_backend? > } > > void vhost_zerocopy_callback(void *arg) > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring 2011-07-19 19:49 ` Michael S. Tsirkin @ 2011-07-19 20:50 ` Shirley Ma 2011-07-20 10:41 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Shirley Ma @ 2011-07-19 20:50 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang 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: > > 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<xma@us.ibm.com> > > 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. > > --- > > > > 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); > > 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. > > > + > > + /* 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); > > 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). ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring 2011-07-19 20:50 ` Shirley Ma @ 2011-07-20 10:41 ` Michael S. Tsirkin 2011-07-20 18:12 ` Shirley Ma 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2011-07-20 10:41 UTC (permalink / raw) To: Shirley Ma; +Cc: David Miller, netdev, jasowang 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: > > 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<xma@us.ibm.com> > > > > 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. > > > > > + > > > + /* 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); > > > > 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring 2011-07-20 10:41 ` Michael S. Tsirkin @ 2011-07-20 18:12 ` Shirley Ma 0 siblings, 0 replies; 5+ messages in thread From: Shirley Ma @ 2011-07-20 18:12 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang 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: > > 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: > > > > 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<xma@us.ibm.com> > > > > > > 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. >From the test results, below patch solves the problem. You can check in this. > > > > 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. 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. > > 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. > > > > > > > > + > > > > + /* 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); > > > > > > 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); > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-07-20 18:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-19 18:02 [PATCH] vhost: clean up outstanding buffers before setting vring Shirley Ma 2011-07-19 19:49 ` Michael S. Tsirkin 2011-07-19 20:50 ` Shirley Ma 2011-07-20 10:41 ` Michael S. Tsirkin 2011-07-20 18:12 ` Shirley Ma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).