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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Fixes: 09177ac91921 ("vfio/mtty: Convert to use vfio_register_group_dev()")
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > 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
