diff mbox series

[1/2] vfio: Remove the vfio_group->users and users_comp

Message ID 1-v1-917e3647f123+b1a-vfio_group_users_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Simplify some of the locking in vfio_group | expand

Commit Message

Jason Gunthorpe Sept. 29, 2022, 2:59 p.m. UTC
Kevin points out that the users is really just tracking if
group->opened_file is set, so we can simplify this code to a wait_queue
that looks for !opened_file under the group_rwsem.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.h      |  3 +--
 drivers/vfio/vfio_main.c | 45 +++++++++++++++++-----------------------
 2 files changed, 20 insertions(+), 28 deletions(-)

Comments

Tian, Kevin Sept. 30, 2022, 8:58 a.m. UTC | #1
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, September 29, 2022 10:59 PM
> 
> Kevin points out that the users is really just tracking if
> group->opened_file is set, so we can simplify this code to a wait_queue
> that looks for !opened_file under the group_rwsem.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff mbox series

Patch

diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 039e3208d286fa..78b362a9250113 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -48,8 +48,6 @@  struct vfio_group {
 	 * reaches 0 then the iommu_group is invalid.
 	 */
 	refcount_t			drivers;
-	refcount_t			users;
-	struct completion		users_comp;
 	unsigned int			container_users;
 	struct iommu_group		*iommu_group;
 	struct vfio_container		*container;
@@ -61,6 +59,7 @@  struct vfio_group {
 	struct rw_semaphore		group_rwsem;
 	struct kvm			*kvm;
 	struct file			*opened_file;
+	struct swait_queue_head		opened_file_wait;
 	struct blocking_notifier_head	notifier;
 };
 
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index f19171cad9a25f..57a7576a96a61b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -186,10 +186,9 @@  static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
 	cdev_init(&group->cdev, &vfio_group_fops);
 	group->cdev.owner = THIS_MODULE;
 
-	refcount_set(&group->users, 1);
 	refcount_set(&group->drivers, 1);
-	init_completion(&group->users_comp);
 	init_rwsem(&group->group_rwsem);
+	init_swait_queue_head(&group->opened_file_wait);
 	INIT_LIST_HEAD(&group->device_list);
 	mutex_init(&group->device_lock);
 	group->iommu_group = iommu_group;
@@ -245,12 +244,6 @@  static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
 	return ret;
 }
 
-static void vfio_group_put(struct vfio_group *group)
-{
-	if (refcount_dec_and_test(&group->users))
-		complete(&group->users_comp);
-}
-
 static void vfio_device_remove_group(struct vfio_device *device)
 {
 	struct vfio_group *group = device->group;
@@ -270,10 +263,6 @@  static void vfio_device_remove_group(struct vfio_device *device)
 	 * cdev_device_add() will fail due to the name aready existing.
 	 */
 	cdev_device_del(&group->cdev, &group->dev);
-	mutex_unlock(&vfio.group_lock);
-
-	/* Matches the get from vfio_group_alloc() */
-	vfio_group_put(group);
 
 	/*
 	 * Before we allow the last driver in the group to be unplugged the
@@ -281,7 +270,13 @@  static void vfio_device_remove_group(struct vfio_device *device)
 	 * is because the group->iommu_group pointer should only be used so long
 	 * as a device driver is attached to a device in the group.
 	 */
-	wait_for_completion(&group->users_comp);
+	while (group->opened_file) {
+		mutex_unlock(&vfio.group_lock);
+		swait_event_idle_exclusive(group->opened_file_wait,
+					   !group->opened_file);
+		mutex_lock(&vfio.group_lock);
+	}
+	mutex_unlock(&vfio.group_lock);
 
 	/*
 	 * These data structures all have paired operations that can only be
@@ -906,15 +901,18 @@  static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 
 	down_write(&group->group_rwsem);
 
-	/* users can be zero if this races with vfio_device_remove_group() */
-	if (!refcount_inc_not_zero(&group->users)) {
+	/*
+	 * drivers can be zero if this races with vfio_device_remove_group(), it
+	 * will be stable at 0 under the group rwsem
+	 */
+	if (refcount_read(&group->drivers) == 0) {
 		ret = -ENODEV;
-		goto err_unlock;
+		goto out_unlock;
 	}
 
 	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO)) {
 		ret = -EPERM;
-		goto err_put;
+		goto out_unlock;
 	}
 
 	/*
@@ -922,16 +920,12 @@  static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 	 */
 	if (group->opened_file) {
 		ret = -EBUSY;
-		goto err_put;
+		goto out_unlock;
 	}
 	group->opened_file = filep;
 	filep->private_data = group;
-
-	up_write(&group->group_rwsem);
-	return 0;
-err_put:
-	vfio_group_put(group);
-err_unlock:
+	ret = 0;
+out_unlock:
 	up_write(&group->group_rwsem);
 	return ret;
 }
@@ -952,8 +946,7 @@  static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 		vfio_group_detach_container(group);
 	group->opened_file = NULL;
 	up_write(&group->group_rwsem);
-
-	vfio_group_put(group);
+	swake_up_one(&group->opened_file_wait);
 
 	return 0;
 }