On Tue, 2014-06-03 at 00:30 +0300, Michael S. Tsirkin wrote:> All memory accesses are done under some VQ mutex. > So lock/unlock all VQs is a faster equivalent of synchronize_rcu() > for memory access changes. > Some guests cause a lot of these changes, so it's helpful > to make them faster. > > Reported-by: "Gonglei (Arei)" <arei.gonglei at huawei.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/vhost/vhost.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 78987e4..1c05e60 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -593,6 +593,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > { > struct vhost_memory mem, *newmem, *oldmem; > unsigned long size = offsetof(struct vhost_memory, regions); > + int i; > > if (copy_from_user(&mem, m, size)) > return -EFAULT; > @@ -619,7 +620,14 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > oldmem = rcu_dereference_protected(d->memory, > lockdep_is_held(&d->mutex)); > rcu_assign_pointer(d->memory, newmem); > - synchronize_rcu(); > + > + /* All memory accesses are done under some VQ mutex. > + * So below is a faster equivalent of synchronize_rcu() > + */ > + for (i = 0; i < d->nvqs; ++i) { > + mutex_lock(&d->vqs[i]->mutex); > + mutex_unlock(&d->vqs[i]->mutex); > + } > kfree(oldmem); > return 0; > }This looks dubious What about using kfree_rcu() instead ? translate_desc() still uses rcu_read_lock(), its not clear if the mutex is really held.
Il 02/06/2014 23:58, Eric Dumazet ha scritto:> This looks dubious > > What about using kfree_rcu() instead ?It would lead to unbound allocation from userspace.> translate_desc() still uses rcu_read_lock(), its not clear if the mutex > is really held.Yes, vhost_get_vq_desc must be called with the vq mutex held. The rcu_read_lock/unlock in translate_desc is unnecessary. Paolo
On 06/03/2014 08:48 AM, Paolo Bonzini wrote:> Il 02/06/2014 23:58, Eric Dumazet ha scritto: >> This looks dubious >> >> What about using kfree_rcu() instead ? > > It would lead to unbound allocation from userspace. > >> translate_desc() still uses rcu_read_lock(), its not clear if the mutex >> is really held. > > Yes, vhost_get_vq_desc must be called with the vq mutex held. > > The rcu_read_lock/unlock in translate_desc is unnecessary. >If that's true, then does dev->memory really needs to be rcu protected? It appears to always be read under mutex. -vlad> Paolo > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-06-03 at 14:48 +0200, Paolo Bonzini wrote:> Il 02/06/2014 23:58, Eric Dumazet ha scritto: > > This looks dubious > > > > What about using kfree_rcu() instead ? > > It would lead to unbound allocation from userspace.Look at how we did this in commit c3059477fce2d956a0bb3e04357324780c5d8eeb> > > translate_desc() still uses rcu_read_lock(), its not clear if the mutex > > is really held. > > Yes, vhost_get_vq_desc must be called with the vq mutex held. > > The rcu_read_lock/unlock in translate_desc is unnecessary.Yep, this is what I pointed out. This is not only necessary, but confusing and might be incorrectly copy/pasted in the future. This patch is a partial one and leaves confusion. Some places uses the proper mp = rcu_dereference_protected(dev->memory, lockdep_is_held(&dev->mutex)); others use the now incorrect : rcu_read_lock(); mp = rcu_dereference(dev->memory); ...
On Mon, Jun 02, 2014 at 02:58:00PM -0700, Eric Dumazet wrote:> On Tue, 2014-06-03 at 00:30 +0300, Michael S. Tsirkin wrote: > > All memory accesses are done under some VQ mutex. > > So lock/unlock all VQs is a faster equivalent of synchronize_rcu() > > for memory access changes. > > Some guests cause a lot of these changes, so it's helpful > > to make them faster. > > > > Reported-by: "Gonglei (Arei)" <arei.gonglei at huawei.com> > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > drivers/vhost/vhost.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index 78987e4..1c05e60 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -593,6 +593,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > > { > > struct vhost_memory mem, *newmem, *oldmem; > > unsigned long size = offsetof(struct vhost_memory, regions); > > + int i; > > > > if (copy_from_user(&mem, m, size)) > > return -EFAULT; > > @@ -619,7 +620,14 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > > oldmem = rcu_dereference_protected(d->memory, > > lockdep_is_held(&d->mutex)); > > rcu_assign_pointer(d->memory, newmem); > > - synchronize_rcu(); > > + > > + /* All memory accesses are done under some VQ mutex. > > + * So below is a faster equivalent of synchronize_rcu() > > + */ > > + for (i = 0; i < d->nvqs; ++i) { > > + mutex_lock(&d->vqs[i]->mutex); > > + mutex_unlock(&d->vqs[i]->mutex); > > + } > > kfree(oldmem); > > return 0; > > } > > This looks dubious > > What about using kfree_rcu() instead ?Unfortunately userspace relies on the fact that no one uses the old mappings by the time ioctl returns. The issue isn't freeing the memory.> translate_desc() still uses rcu_read_lock(), its not clear if the mutex > is really held. >Thanks, good point, we can drop that rcu_read_lock now, but I think this could be a patch on top. -- MST