Re: [Qemu-devel] [RFC] vhost: Can we change synchronize_rcu to call_rcu in vhost_set_memory() in vhost kernel module?

[Michael S. Tsirkin]

On Tue, May 13, 2014 at 07:03:20AM +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:address@hidden
> > Sent: Monday, May 12, 2014 6:31 PM
> > 
> > vhost does everything under a VQ lock.
> > I think RCU for VHOST_SET_MEM_TABLE can be replaced with
> > taking and freeing the VQ lock.
> > 
> > Does the below solve the problem for you
> > (warning: untested, sorry, busy with other bugs right now)?
> > 
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > 
> > ---
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 78987e4..df2e3eb 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 < dev->nvqs; ++i) {
> > +           mutex_lock(d->vqs[idx]->mutex);
> > +           mutex_unlock(d->vqs[idx]->mutex);
> > +   }
> >     kfree(oldmem);
> >     return 0;
> >  }
> 
> Thanks for your advice, I suppose getting mutexes should generally be faster 
> than waiting for 
> CPU context switches. And I think d->mutex should also be synchronized since 
> somewhere gets 
> only this mutex directly and not vq mutexes. Is this right?

No because all memory table accesses are under some vq mutex.

> I'll try this approach, thanks.
> 
> Best regards,
> -Gonglei