[PATCH] vfio/mtty: Delete mdev_devices_list

[PATCH] vfio/mtty: Delete mdev_devices_list

[Jason Gunthorpe]

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
--- a/samples/vfio-mdev/mtty.c
+++ 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);
+
 	dev_set_drvdata(&mdev->dev, mdev_state);
 	return 0;
 }
@@ -750,10 +747,8 @@ static void mtty_remove(struct mdev_device *mdev)
 {
 	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
 
+	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
 	vfio_unregister_group_dev(&mdev_state->vdev);
-	mutex_lock(&mdev_list_lock);
-	list_del(&mdev_state->next);
-	mutex_unlock(&mdev_list_lock);
 
 	kfree(mdev_state->vconfig);
 	kfree(mdev_state);
@@ -1274,14 +1269,10 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
 {
-	struct mdev_state *mds;
 	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
-	int used = 0;
 
-	list_for_each_entry(mds, &mdev_devices_list, next)
-		used += mds->nr_ports;
-
-	return sprintf(buf, "%d\n", (MAX_MTTYS - used)/ports);
+	return sprintf(buf, "%d\n",
+		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
 }
 
 static MDEV_TYPE_ATTR_RO(available_instances);
@@ -1395,9 +1386,6 @@ static int __init mtty_dev_init(void)
 	ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);
 	if (ret)
 		goto err_device;
-
-	mutex_init(&mdev_list_lock);
-	INIT_LIST_HEAD(&mdev_devices_list);
 	return 0;
 
 err_device:
-- 
2.32.0

Re: [PATCH] vfio/mtty: Delete mdev_devices_list

[Alex Williamson]

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
> --- a/samples/vfio-mdev/mtty.c
> +++ 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 :-\  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.  I don't know if there's a better solution than:

+       for (i = nr_ports; i; i--) {
+               if (!atomic_add_unless(&available_ports, -1, 0))
+                       break;
+       }
+       if (i) {
+               atomic_add(nr_ports - i, &available_ports);
+               return -ENOSPC;
+       }

Thanks,
Alex

>  	dev_set_drvdata(&mdev->dev, mdev_state);
>  	return 0;
>  }
> @@ -750,10 +747,8 @@ static void mtty_remove(struct mdev_device *mdev)
>  {
>  	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
>  
> +	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
>  	vfio_unregister_group_dev(&mdev_state->vdev);
> -	mutex_lock(&mdev_list_lock);
> -	list_del(&mdev_state->next);
> -	mutex_unlock(&mdev_list_lock);
>  
>  	kfree(mdev_state->vconfig);
>  	kfree(mdev_state);
> @@ -1274,14 +1269,10 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>  					struct mdev_type_attribute *attr,
>  					char *buf)
>  {
> -	struct mdev_state *mds;
>  	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
> -	int used = 0;
>  
> -	list_for_each_entry(mds, &mdev_devices_list, next)
> -		used += mds->nr_ports;
> -
> -	return sprintf(buf, "%d\n", (MAX_MTTYS - used)/ports);
> +	return sprintf(buf, "%d\n",
> +		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
>  }
>  
>  static MDEV_TYPE_ATTR_RO(available_instances);
> @@ -1395,9 +1386,6 @@ static int __init mtty_dev_init(void)
>  	ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);
>  	if (ret)
>  		goto err_device;
> -
> -	mutex_init(&mdev_list_lock);
> -	INIT_LIST_HEAD(&mdev_devices_list);
>  	return 0;
>  
>  err_device:

Re: [PATCH] vfio/mtty: Delete mdev_devices_list

[Jason Gunthorpe]

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

Re: [PATCH] vfio/mtty: Delete mdev_devices_list

[Cornelia Huck]

On Fri, Jun 25 2021, 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(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>