Re: [PATCH v4 2/3] vfio: Add an open counter to vfio_device_set

[Alex Williamson <alex.williamson@redhat.com>]

On Fri,  4 Nov 2022 12:57:26 -0700
Anthony DeRossi <ajderossi@gmail.com> wrote:

> open_count is incremented before open_device() and decremented after
> close_device() for each device in the set. This allows devices to
> determine whether shared resources are in use without tracking them
> manually or accessing the private open_count in vfio_device.
> 
> Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
> ---
>  drivers/vfio/vfio_main.c | 3 +++
>  include/linux/vfio.h     | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 9a4af880e941..6c65418fc7e3 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -761,6 +761,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  		mutex_lock(&device->group->group_lock);
>  		device->kvm = device->group->kvm;
>  
> +		device->dev_set->open_count++;

Note that this is on the device first open path...

>  		if (device->ops->open_device) {
>  			ret = device->ops->open_device(device);
>  			if (ret)
> @@ -809,6 +810,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  	}
>  err_undo_count:
>  	mutex_unlock(&device->group->group_lock);
> +	device->dev_set->open_count--;

But this can be reached for non-first open faults.

>  	device->open_count--;
>  	if (device->open_count == 0 && device->kvm)
>  		device->kvm = NULL;
> @@ -1023,6 +1025,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  			device->ops->close_device(device);
>  
>  		vfio_device_container_unregister(device);
> +		device->dev_set->open_count--;

This is on the last-close path, so aside from the above bug this is more
of an open device counter across the device set rather than than a
number of open device file descriptors counter as we have on each
device.

There's some complexity thinking about the difference between those for
our scenario, it works, but requires a bit of deduction.  For example,
a device set might have a count of 1 here, but that one device could be
opened multiple times.  It works for the scenario you address in the
next patch, but is maybe not as generically useful.

Like it seems maybe you're going for by your more recent comment, I was
thinking an interface rather than tracking a new field on the device
set.  Thanks,

Alex

>  	}
>  	mutex_unlock(&device->group->group_lock);
>  	device->open_count--;
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e7cebeb875dd..5becdcdf4ba2 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -28,6 +28,7 @@ struct vfio_device_set {
>  	struct mutex lock;
>  	struct list_head device_list;
>  	unsigned int device_count;
> +	unsigned int open_count;
>  };
>  
>  struct vfio_device {