Re: [PATCH RFC v2 02/10] vringh: add 'iotlb_lock' to synchronize iotlb accesses

[Jason Wang]

On 2021/1/29 下午5:18, Stefano Garzarella wrote:
> On Fri, Jan 29, 2021 at 03:43:40PM +0800, Jason Wang wrote:
> > On 2021/1/28 下午10:41, Stefano Garzarella wrote:
> > > Usually iotlb accesses are synchronized with a spinlock.
> > > Let's request it as a new parameter in vringh_set_iotlb() and
> > > hold it when we navigate the iotlb in iotlb_translate() to avoid
> > > race conditions with any new additions/deletions of ranges from
> > > the ioltb.
> >
> >
> > Patch looks fine but I wonder if this is the best approach comparing 
> > to do locking by the caller.
>
> Initially I tried to hold the lock in the vdpasim_blk_work(), but since
> we have a lot of different functions for vringh, I opted to take the 
> lock at the beginning and release it at the end.
> Also because several times I went to see if that call used 
> iotlb_translate or not.
>
> This could be a problem for example if we have multiple workers to 
> handle multiple queues.
>
> Also, some functions are quite long (e.g. vringh_getdesc_iotlb) and 
> holding the lock for that long could reduce parallelism.
>
> For these reasons I thought it was better to hide everything from the 
> caller who doesn't have to worry about which function calls 
> iotlb_translate() and thus hold the lock.


Fine with me.

Acked-by: Jason Wang <jasowang@xxxxxxxxxx>

Thanks


> Thanks,
> Stefano
>
> > Thanks
> >
> >
> > > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > > ---
> > >  include/linux/vringh.h           | 6 +++++-
> > >  drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++-
> > >  drivers/vhost/vringh.c           | 9 ++++++++-
> > >  3 files changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > > index 59bd50f99291..9c077863c8f6 100644
> > > --- a/include/linux/vringh.h
> > > +++ b/include/linux/vringh.h
> > > @@ -46,6 +46,9 @@ struct vringh {
> > >      /* IOTLB for this vring */
> > >      struct vhost_iotlb *iotlb;
> > > +    /* spinlock to synchronize IOTLB accesses */
> > > +    spinlock_t *iotlb_lock;
> > > +
> > >      /* The function to call to notify the guest about added buffers */
> > >      void (*notify)(struct vringh *);
> > >  };
> > > @@ -258,7 +261,8 @@ static inline __virtio64 cpu_to_vringh64(const 
> > > struct vringh *vrh, u64 val)
> > >  #if IS_REACHABLE(CONFIG_VHOST_IOTLB)
> > > -void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb);
> > > +void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb,
> > > +              spinlock_t *iotlb_lock);
> > >  int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > >                unsigned int num, bool weak_barriers,
> > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c 
> > > b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > index 2183a833fcf4..53238989713d 100644
> > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > @@ -284,7 +284,8 @@ struct vdpasim *vdpasim_create(struct 
> > > vdpasim_dev_attr *dev_attr)
> > >          goto err_iommu;
> > >      for (i = 0; i < dev_attr->nvqs; i++)
> > > -        vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu);
> > > +        vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu,
> > > +                 &vdpasim->iommu_lock);
> > >      ret = iova_cache_get();
> > >      if (ret)
> > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > index 85d85faba058..f68122705719 100644
> > > --- a/drivers/vhost/vringh.c
> > > +++ b/drivers/vhost/vringh.c
> > > @@ -1074,6 +1074,8 @@ static int iotlb_translate(const struct vringh 
> > > *vrh,
> > >      int ret = 0;
> > >      u64 s = 0;
> > > +    spin_lock(vrh->iotlb_lock);
> > > +
> > >      while (len > s) {
> > >          u64 size, pa, pfn;
> > > @@ -1103,6 +1105,8 @@ static int iotlb_translate(const struct vringh 
> > > *vrh,
> > >          ++ret;
> > >      }
> > > +    spin_unlock(vrh->iotlb_lock);
> > > +
> > >      return ret;
> > >  }
> > > @@ -1262,10 +1266,13 @@ EXPORT_SYMBOL(vringh_init_iotlb);
> > >   * vringh_set_iotlb - initialize a vringh for a ring with IOTLB.
> > >   * @vrh: the vring
> > >   * @iotlb: iotlb associated with this vring
> > > + * @iotlb_lock: spinlock to synchronize the iotlb accesses
> > >   */
> > > -void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb)
> > > +void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb,
> > > +              spinlock_t *iotlb_lock)
> > >  {
> > >      vrh->iotlb = iotlb;
> > > +    vrh->iotlb_lock = iotlb_lock;
> > >  }
> > >  EXPORT_SYMBOL(vringh_set_iotlb);