Re: [PATCH 1/4] vfio: Simplify vfio_create_group()

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 9/8/22 2:44 PM, Jason Gunthorpe wrote:
> The vfio.group_lock is now only used to serialize vfio_group creation
> and destruction, we don't need a micro-optimization of searching,
> unlocking, then allocating and searching again. Just hold the lock
> the whole time.
> 
> Rename the function to 'vfio_get_group()' to reflect that it doesn't
> always create something.
> move
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  drivers/vfio/vfio_main.c | 48 ++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 77264d836d5200..4ab13808b536e1 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -319,17 +319,6 @@ __vfio_group_get_from_iommu(struct iommu_group *iommu_group)
>  	return NULL;
>  }
>  
> -static struct vfio_group *
> -vfio_group_get_from_iommu(struct iommu_group *iommu_group)
> -{
> -	struct vfio_group *group;
> -
> -	mutex_lock(&vfio.group_lock);
> -	group = __vfio_group_get_from_iommu(iommu_group);
> -	mutex_unlock(&vfio.group_lock);
> -	return group;
> -}
> -
>  static void vfio_group_release(struct device *dev)
>  {
>  	struct vfio_group *group = container_of(dev, struct vfio_group, dev);
> @@ -376,16 +365,26 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
>  	return group;
>  }
>  
> -static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> -		enum vfio_group_type type)
> +/*
> + * Return a struct vfio_group * for the given iommu_group. If no vfio_group
> + * already exists then create a new one.
> + */
> +static struct vfio_group *vfio_get_group(struct iommu_group *iommu_group,
> +					 enum vfio_group_type type)
>  {
>  	struct vfio_group *group;
>  	struct vfio_group *ret;
>  	int err;
>  
> -	group = vfio_group_alloc(iommu_group, type);
> -	if (IS_ERR(group))
> -		return group;
> +	mutex_lock(&vfio.group_lock);
> +
> +	ret = __vfio_group_get_from_iommu(iommu_group);
> +	if (ret)
> +		goto err_unlock;
> +
> +	group = ret = vfio_group_alloc(iommu_group, type);
> +	if (IS_ERR(ret))
> +		goto err_unlock;
>  
>  	err = dev_set_name(&group->dev, "%s%d",
>  			   group->type == VFIO_NO_IOMMU ? "noiommu-" : "",
> @@ -395,13 +394,6 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
>  		goto err_put;
>  	}
>  
> -	mutex_lock(&vfio.group_lock);
> -
> -	/* Did we race creating this group? */
> -	ret = __vfio_group_get_from_iommu(iommu_group);
> -	if (ret)
> -		goto err_unlock;
> -
>  	err = cdev_device_add(&group->cdev, &group->dev);
>  	if (err) {
>  		ret = ERR_PTR(err);
> @@ -413,10 +405,10 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
>  	mutex_unlock(&vfio.group_lock);
>  	return group;
>  
> -err_unlock:
> -	mutex_unlock(&vfio.group_lock);
>  err_put:
>  	put_device(&group->dev);
> +err_unlock:
> +	mutex_unlock(&vfio.group_lock);
>  	return ret;
>  }
>  
> @@ -514,7 +506,7 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
>  	if (ret)
>  		goto out_put_group;
>  
> -	group = vfio_create_group(iommu_group, type);
> +	group = vfio_get_group(iommu_group, type);
>  	if (IS_ERR(group)) {
>  		ret = PTR_ERR(group);
>  		goto out_remove_device;
> @@ -564,9 +556,7 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	group = vfio_group_get_from_iommu(iommu_group);
> -	if (!group)
> -		group = vfio_create_group(iommu_group, VFIO_IOMMU);
> +	group = vfio_get_group(iommu_group, VFIO_IOMMU);
>  
>  	/* The vfio_group holds a reference to the iommu_group */
>  	iommu_group_put(iommu_group);