Re: [PATCH] vfio/mtty: Delete mdev_devices_list

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jun 25, 2021 at 10:26:20AM -0600, Alex Williamson wrote:
> On Fri, 25 Jun 2021 12:56:04 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > Dan points out that an error case left things on this list. It is also
> > missing locking in available_instances_show().
> > 
> > Further study shows the list isn't needed at all, just store the total
> > ports in use in an atomic and delete the whole thing.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Fixes: 09177ac91921 ("vfio/mtty: Convert to use vfio_register_group_dev()")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  samples/vfio-mdev/mtty.c | 24 ++++++------------------
> >  1 file changed, 6 insertions(+), 18 deletions(-)
> > 
> > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> > index faf9b8e8873a5b..ffbaf07a17eaee 100644
> > +++ b/samples/vfio-mdev/mtty.c
> > @@ -144,8 +144,7 @@ struct mdev_state {
> >  	int nr_ports;
> >  };
> >  
> > -static struct mutex mdev_list_lock;
> > -static struct list_head mdev_devices_list;
> > +static atomic_t mdev_used_ports;
> >  
> >  static const struct file_operations vd_fops = {
> >  	.owner          = THIS_MODULE,
> > @@ -733,15 +732,13 @@ static int mtty_probe(struct mdev_device *mdev)
> >  
> >  	mtty_create_config_space(mdev_state);
> >  
> > -	mutex_lock(&mdev_list_lock);
> > -	list_add(&mdev_state->next, &mdev_devices_list);
> > -	mutex_unlock(&mdev_list_lock);
> > -
> >  	ret = vfio_register_group_dev(&mdev_state->vdev);
> >  	if (ret) {
> >  		kfree(mdev_state);
> >  		return ret;
> >  	}
> > +	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
> > +
> 
> I was just looking at the same and noticing how available_instances is
> not enforced :-\

I saw that too - it is something someone could do on top of this
atomic change without too much trouble. I'm not sure it is worthwhile
to work on these samples too much

> What if we ATOMIC_INIT(MAX_TTYS) and use this as available ports
> rather than used ports.  We can check and return -ENOSPC at the
> beginning or probe if we can't allocate the ports.  The only
> complication is when we need to atomically subtract >1 and not go
> negative.

It is usually done with a cmpxchg loop:

static inline int atomic_sub_if_positive(int i, atomic_t *v)
{
        int dec, c = atomic_read(v);

        do {
                dec = c - i;
                if (unlikely(dec < 0))
                        break;
        } while (!atomic_try_cmpxchg(v, &c, dec));

        return dec;
}

Or even a simple logic with atomic_sub_return() would do well enough
for this.

Jason