| Message ID | 0-v1-0bc56b362ca7+62-mtty_used_ports_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mtty: Delete mdev_devices_list | expand |
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:
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
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>
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:
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(-)