[PATCH 0/2] Simplify some of the locking in vfio_group

[PATCH 0/2] Simplify some of the locking in vfio_group

[Jason Gunthorpe]

Kevin points out that the vfio_group->users doesn't really need to exist
now, and some other inspection shows that the group_rwsem has outlived its
utility as well. Replace both with simpler constructs.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (2):
  vfio: Remove the vfio_group->users and users_comp
  vfio: Change vfio_group->group_rwsem to a mutex

 drivers/vfio/container.c | 10 ++---
 drivers/vfio/vfio.h      |  5 +--
 drivers/vfio/vfio_main.c | 92 +++++++++++++++++++---------------------
 3 files changed, 50 insertions(+), 57 deletions(-)


base-commit: 42e1d1eed20a17c6cbb1d600c77a6ca69a632d4c
-- 
2.37.3

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

[Jason Gunthorpe]

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

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;
 }
-- 
2.37.3

[PATCH 2/2] vfio: Change vfio_group->group_rwsem to a mutex

[Jason Gunthorpe]

These days not much is using the read side:
 - device first open
 - ioctl_get_status
 - device FD release
 - check enforced_coherent

None of this is performance, so just make it into a normal mutex.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/container.c | 10 ++++-----
 drivers/vfio/vfio.h      |  2 +-
 drivers/vfio/vfio_main.c | 47 ++++++++++++++++++++--------------------
 3 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index db7c071ee3de1a..d74164abbf401d 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -430,7 +430,7 @@ int vfio_container_attach_group(struct vfio_container *container,
 	struct vfio_iommu_driver *driver;
 	int ret = 0;
 
-	lockdep_assert_held_write(&group->group_rwsem);
+	lockdep_assert_held(&group->group_lock);
 
 	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
 		return -EPERM;
@@ -481,7 +481,7 @@ void vfio_group_detach_container(struct vfio_group *group)
 	struct vfio_container *container = group->container;
 	struct vfio_iommu_driver *driver;
 
-	lockdep_assert_held_write(&group->group_rwsem);
+	lockdep_assert_held(&group->group_lock);
 	WARN_ON(group->container_users != 1);
 
 	down_write(&container->group_lock);
@@ -515,7 +515,7 @@ int vfio_device_assign_container(struct vfio_device *device)
 {
 	struct vfio_group *group = device->group;
 
-	lockdep_assert_held_write(&group->group_rwsem);
+	lockdep_assert_held(&group->group_lock);
 
 	if (!group->container || !group->container->iommu_driver ||
 	    WARN_ON(!group->container_users))
@@ -531,11 +531,11 @@ int vfio_device_assign_container(struct vfio_device *device)
 
 void vfio_device_unassign_container(struct vfio_device *device)
 {
-	down_write(&device->group->group_rwsem);
+	mutex_lock(&device->group->group_lock);
 	WARN_ON(device->group->container_users <= 1);
 	device->group->container_users--;
 	fput(device->group->opened_file);
-	up_write(&device->group->group_rwsem);
+	mutex_unlock(&device->group->group_lock);
 }
 
 /*
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 78b362a9250113..4a1bac1359a952 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -56,7 +56,7 @@ struct vfio_group {
 	struct list_head		vfio_next;
 	struct list_head		container_next;
 	enum vfio_group_type		type;
-	struct rw_semaphore		group_rwsem;
+	struct mutex			group_lock;
 	struct kvm			*kvm;
 	struct file			*opened_file;
 	struct swait_queue_head		opened_file_wait;
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 57a7576a96a61b..9207e6c0e3cb26 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -158,6 +158,7 @@ static void vfio_group_release(struct device *dev)
 	struct vfio_group *group = container_of(dev, struct vfio_group, dev);
 
 	mutex_destroy(&group->device_lock);
+	mutex_destroy(&group->group_lock);
 	iommu_group_put(group->iommu_group);
 	ida_free(&vfio.group_ida, MINOR(group->dev.devt));
 	kfree(group);
@@ -187,7 +188,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
 	group->cdev.owner = THIS_MODULE;
 
 	refcount_set(&group->drivers, 1);
-	init_rwsem(&group->group_rwsem);
+	mutex_init(&group->group_lock);
 	init_swait_queue_head(&group->opened_file_wait);
 	INIT_LIST_HEAD(&group->device_list);
 	mutex_init(&group->device_lock);
@@ -665,7 +666,7 @@ static int vfio_group_ioctl_unset_container(struct vfio_group *group)
 {
 	int ret = 0;
 
-	down_write(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	if (!group->container) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -677,7 +678,7 @@ static int vfio_group_ioctl_unset_container(struct vfio_group *group)
 	vfio_group_detach_container(group);
 
 out_unlock:
-	up_write(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 	return ret;
 }
 
@@ -696,7 +697,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
 	if (!f.file)
 		return -EBADF;
 
-	down_write(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	if (group->container || WARN_ON(group->container_users)) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -709,7 +710,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
 	}
 
 out_unlock:
-	up_write(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 	fdput(f);
 	return ret;
 }
@@ -727,9 +728,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
 	struct file *filep;
 	int ret;
 
-	down_write(&device->group->group_rwsem);
+	mutex_lock(&device->group->group_lock);
 	ret = vfio_device_assign_container(device);
-	up_write(&device->group->group_rwsem);
+	mutex_unlock(&device->group->group_lock);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -746,7 +747,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
 		 * lock.  If the device driver will use it, it must obtain a
 		 * reference and release it during close_device.
 		 */
-		down_read(&device->group->group_rwsem);
+		mutex_lock(&device->group->group_lock);
 		device->kvm = device->group->kvm;
 
 		if (device->ops->open_device) {
@@ -755,7 +756,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
 				goto err_undo_count;
 		}
 		vfio_device_container_register(device);
-		up_read(&device->group->group_rwsem);
+		mutex_unlock(&device->group->group_lock);
 	}
 	mutex_unlock(&device->dev_set->lock);
 
@@ -788,14 +789,14 @@ static struct file *vfio_device_open(struct vfio_device *device)
 
 err_close_device:
 	mutex_lock(&device->dev_set->lock);
-	down_read(&device->group->group_rwsem);
+	mutex_lock(&device->group->group_lock);
 	if (device->open_count == 1 && device->ops->close_device) {
 		device->ops->close_device(device);
 
 		vfio_device_container_unregister(device);
 	}
 err_undo_count:
-	up_read(&device->group->group_rwsem);
+	mutex_unlock(&device->group->group_lock);
 	device->open_count--;
 	if (device->open_count == 0 && device->kvm)
 		device->kvm = NULL;
@@ -860,13 +861,13 @@ static int vfio_group_ioctl_get_status(struct vfio_group *group,
 
 	status.flags = 0;
 
-	down_read(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	if (group->container)
 		status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET |
 				VFIO_GROUP_FLAGS_VIABLE;
 	else if (!iommu_group_dma_owner_claimed(group->iommu_group))
 		status.flags |= VFIO_GROUP_FLAGS_VIABLE;
-	up_read(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 
 	if (copy_to_user(arg, &status, minsz))
 		return -EFAULT;
@@ -899,7 +900,7 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 		container_of(inode->i_cdev, struct vfio_group, cdev);
 	int ret;
 
-	down_write(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 
 	/*
 	 * drivers can be zero if this races with vfio_device_remove_group(), it
@@ -926,7 +927,7 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 	filep->private_data = group;
 	ret = 0;
 out_unlock:
-	up_write(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 	return ret;
 }
 
@@ -936,7 +937,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 
 	filep->private_data = NULL;
 
-	down_write(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	/*
 	 * Device FDs hold a group file reference, therefore the group release
 	 * is only called when there are no open devices.
@@ -945,7 +946,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 	if (group->container)
 		vfio_group_detach_container(group);
 	group->opened_file = NULL;
-	up_write(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 	swake_up_one(&group->opened_file_wait);
 
 	return 0;
@@ -1001,12 +1002,12 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
-	down_read(&device->group->group_rwsem);
+	mutex_lock(&device->group->group_lock);
 	if (device->open_count == 1 && device->ops->close_device)
 		device->ops->close_device(device);
 
 	vfio_device_container_unregister(device);
-	up_read(&device->group->group_rwsem);
+	mutex_unlock(&device->group->group_lock);
 	device->open_count--;
 	if (device->open_count == 0)
 		device->kvm = NULL;
@@ -1580,7 +1581,7 @@ bool vfio_file_enforced_coherent(struct file *file)
 	if (file->f_op != &vfio_group_fops)
 		return true;
 
-	down_read(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	if (group->container) {
 		ret = vfio_container_ioctl_check_extension(group->container,
 							   VFIO_DMA_CC_IOMMU);
@@ -1592,7 +1593,7 @@ bool vfio_file_enforced_coherent(struct file *file)
 		 */
 		ret = true;
 	}
-	up_read(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
@@ -1612,9 +1613,9 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
 	if (file->f_op != &vfio_group_fops)
 		return;
 
-	down_write(&group->group_rwsem);
+	mutex_lock(&group->group_lock);
 	group->kvm = kvm;
-	up_write(&group->group_rwsem);
+	mutex_unlock(&group->group_lock);
 }
 EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
 
-- 
2.37.3

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

[Tian, Kevin]

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

RE: [PATCH 2/2] vfio: Change vfio_group->group_rwsem to a mutex

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, September 29, 2022 10:59 PM
> 
> These days not much is using the read side:
>  - device first open
>  - ioctl_get_status
>  - device FD release
>  - check enforced_coherent
> 
> None of this is performance, so just make it into a normal mutex.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

Re: [PATCH 0/2] Simplify some of the locking in vfio_group

[Alex Williamson]

On Thu, 29 Sep 2022 11:59:23 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> Kevin points out that the vfio_group->users doesn't really need to exist
> now, and some other inspection shows that the group_rwsem has outlived its
> utility as well. Replace both with simpler constructs.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason Gunthorpe (2):
>   vfio: Remove the vfio_group->users and users_comp
>   vfio: Change vfio_group->group_rwsem to a mutex
> 
>  drivers/vfio/container.c | 10 ++---
>  drivers/vfio/vfio.h      |  5 +--
>  drivers/vfio/vfio_main.c | 92 +++++++++++++++++++---------------------
>  3 files changed, 50 insertions(+), 57 deletions(-)
> 
> 
> base-commit: 42e1d1eed20a17c6cbb1d600c77a6ca69a632d4c

Applied to vfio next branch for v6.1.  Thanks,

Alex