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

[Jason Wang]

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.

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);
>