Re: [PATCH] vfio/mtty: Enforce available_instances

[Kirti Wankhede <kwankhede@xxxxxxxxxx>]

On 6/29/2021 12:26 AM, Alex Williamson wrote:
> On Mon, 28 Jun 2021 23:19:54 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>
> > On 6/26/2021 2:56 AM, Alex Williamson wrote:
> > > The sample mtty mdev driver doesn't actually enforce the number of
> > > device instances it claims are available.  Implement this properly.
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > ---
> > >
> > > Applies to vfio next branch + Jason's atomic conversion
> > >    
> >
> >
> > Does this need to be on top of Jason's patch?
>
> Yes, see immediately above.
>
> > Patch to use mdev_used_ports is reverted here, can it be changed from
> > mdev_devices_list to mdev_avail_ports atomic variable?
>
> It doesn't revert Jason's change, it builds on it.  The patches could
> we squashed, but there's no bug in Jason's patch that we're trying to
> avoid exposing, so I don't see why we'd do that.

'Squashed' is the correct word that 'revert', my bad.

> > Change here to use atomic variable looks good to me.
> >
> > Reviewed by: Kirti Wankhede <kwankhede@xxxxxxxxxx>
>
> Thanks!  It was Jason's patch[1] that converted to use an atomic
> though, so I'm slightly confused if this R-b is for the patch below,
> Jason's patch, or both.  Thanks,

I liked 'mdev_avail_ports' approach than 'mdev_used_ports' approach 
here. This R-b is for below patch.

Thanks,
Kirti

> Alex
>
> [1]https://lore.kernel.org/kvm/0-v1-0bc56b362ca7+62-mtty_used_ports_jgg@xxxxxxxxxx/
>
> > >    samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
> > >    1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> > > index ffbaf07a17ea..8b26fecc4afe 100644
> > > --- a/samples/vfio-mdev/mtty.c
> > > +++ b/samples/vfio-mdev/mtty.c
> > > @@ -144,7 +144,7 @@ struct mdev_state {
> > >    	int nr_ports;
> > >    };
> > >    
> > > -static atomic_t mdev_used_ports;
> > > +static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
> > >    
> > >    static const struct file_operations vd_fops = {
> > >    	.owner          = THIS_MODULE,
> > > @@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
> > >    {
> > >    	struct mdev_state *mdev_state;
> > >    	int nr_ports = mdev_get_type_group_id(mdev) + 1;
> > > +	int avail_ports = atomic_read(&mdev_avail_ports);
> > >    	int ret;
> > >    
> > > +	do {
> > > +		if (avail_ports < nr_ports)
> > > +			return -ENOSPC;
> > > +	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
> > > +				     &avail_ports, avail_ports - nr_ports));
> > > +
> > >    	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
> > > -	if (mdev_state == NULL)
> > > +	if (mdev_state == NULL) {
> > > +		atomic_add(nr_ports, &mdev_avail_ports);
> > >    		return -ENOMEM;
> > > +	}
> > >    
> > >    	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
> > >    
> > > @@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
> > >    
> > >    	if (mdev_state->vconfig == NULL) {
> > >    		kfree(mdev_state);
> > > +		atomic_add(nr_ports, &mdev_avail_ports);
> > >    		return -ENOMEM;
> > >    	}
> > >    
> > > @@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
> > >    	ret = vfio_register_group_dev(&mdev_state->vdev);
> > >    	if (ret) {
> > >    		kfree(mdev_state);
> > > +		atomic_add(nr_ports, &mdev_avail_ports);
> > >    		return ret;
> > >    	}
> > > -	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
> > >    
> > >    	dev_set_drvdata(&mdev->dev, mdev_state);
> > >    	return 0;
> > > @@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
> > >    static void mtty_remove(struct mdev_device *mdev)
> > >    {
> > >    	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
> > > +	int nr_ports = mdev_state->nr_ports;
> > >    
> > > -	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
> > >    	vfio_unregister_group_dev(&mdev_state->vdev);
> > >    
> > >    	kfree(mdev_state->vconfig);
> > >    	kfree(mdev_state);
> > > +	atomic_add(nr_ports, &mdev_avail_ports);
> > >    }
> > >    
> > >    static int mtty_reset(struct mdev_state *mdev_state)
> > > @@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
> > >    {
> > >    	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
> > >    
> > > -	return sprintf(buf, "%d\n",
> > > -		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
> > > +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
> > >    }
> > >    
> > >    static MDEV_TYPE_ATTR_RO(available_instances);
> > >
> > >