diff mbox series

[1/4] vfio: Simplify vfio_create_group()

Message ID 1-v1-ef00ffecea52+2cb-iommu_group_lifetime_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Fix splats releated to using the iommu_group after destroying devices | expand

Commit Message

Jason Gunthorpe Sept. 8, 2022, 6:44 p.m. UTC 
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.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio_main.c | 48 ++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

Comments

Matthew Rosato Sept. 20, 2022, 7:45 p.m. UTC | #1 
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);
diff mbox series

Patch

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