> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Wednesday, September 28, 2022 11:26 PM
>
> 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
num_devices has been removed by one of your patches.
> of drivers that have called 'register' on the group. This also happens
> to be the number of vfio_devices of course.
there could be one driver calling 'register' on multiple groups. That is
the part which gets confusing. If talking about driver it is more accurately
to be 'driver_registrations' then the implication is still about device. 😊
>
> > > 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
a followup patch like below sounds good
>
> 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;
> }
