* [PATCH] vfio/mtty: Delete mdev_devices_list
@ 2021-06-25 15:56 Jason Gunthorpe
2021-06-25 16:26 ` Alex Williamson
2021-06-28 14:20 ` Cornelia Huck
0 siblings, 2 replies; 4+ messages in thread
From: Jason Gunthorpe @ 2021-06-25 15:56 UTC (permalink / raw)
To: kvm, Alex Williamson; +Cc: Dan Carpenter
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/mtty: Delete mdev_devices_list
2021-06-25 15:56 [PATCH] vfio/mtty: Delete mdev_devices_list Jason Gunthorpe
@ 2021-06-25 16:26 ` Alex Williamson
2021-06-25 16:51 ` Jason Gunthorpe
2021-06-28 14:20 ` Cornelia Huck
1 sibling, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2021-06-25 16:26 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: kvm, Dan Carpenter
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:
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/mtty: Delete mdev_devices_list
2021-06-25 16:26 ` Alex Williamson
@ 2021-06-25 16:51 ` Jason Gunthorpe
0 siblings, 0 replies; 4+ messages in thread
From: Jason Gunthorpe @ 2021-06-25 16:51 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, Dan Carpenter
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/mtty: Delete mdev_devices_list
2021-06-25 15:56 [PATCH] vfio/mtty: Delete mdev_devices_list Jason Gunthorpe
2021-06-25 16:26 ` Alex Williamson
@ 2021-06-28 14:20 ` Cornelia Huck
1 sibling, 0 replies; 4+ messages in thread
From: Cornelia Huck @ 2021-06-28 14:20 UTC (permalink / raw)
To: Jason Gunthorpe, kvm, Alex Williamson; +Cc: Dan Carpenter
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-28 14:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 15:56 [PATCH] vfio/mtty: Delete mdev_devices_list Jason Gunthorpe
2021-06-25 16:26 ` Alex Williamson
2021-06-25 16:51 ` Jason Gunthorpe
2021-06-28 14:20 ` Cornelia Huck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).