On Wed, Sep 28, 2022 at 03:51:01AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Friday, September 23, 2022 8:06 AM
> >
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 56fab31f8e0ff8..039e3208d286fa 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -41,7 +41,15 @@ enum vfio_group_type {
> > struct vfio_group {
> > struct device dev;
> > struct cdev cdev;
> > + /*
> > + * When drivers is non-zero a driver is attached to the struct device
> > + * that provided the iommu_group and thus the iommu_group is a
> > valid
> > + * pointer. When drivers is 0 the driver is being detached. Once users
> > + * reaches 0 then the iommu_group is invalid.
> > + */
> > + refcount_t drivers;
>
> While I agree all this patch is doing, the notation of 'drivers' here sounds
> a bit confusing IMHO.
Maybe, I picked it because we recently had a num_devices here that was
a different thing. "drivers" comes from the idea that it is the number
of drivers that have called 'register' on the group. This also happens
to be the number of vfio_devices of course.
> > refcount_t users;
> > + struct completion users_comp;
>
> Now the only place poking 'users' is when a group is opened/closed,
> while group->opened_file already plays the guard role. From this
> angle 'users' sounds redundant now?
Oh interesting. I did try to get rid of that thing, but I was thinking
to make it "disassociate" so we didn't have to sleep at all, but SPAPR
messed that up.. It is a good followup patch
So like this:
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;
}
