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

[Stefano Garzarella <sgarzare@redhat.com>]

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.

Thanks,
Stefano

>
>Thanks
>
>
>>
>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>---
>>  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);
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization