On Wed, 13 Oct 2021 14:42:51 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Wed, Oct 13, 2021 at 07:08:47PM +0200, Christoph Hellwig wrote:
> > > +/* returns true if the get was obtained */
> > > +static bool vfio_group_try_get(struct vfio_group *group)
> > > {
> > > + return refcount_inc_not_zero(&group->users);
> > > }
> >
> > Do we even need this helper? Just open coding the refcount_inc_not_zero
> > would seem easier to read to me, and there is just a single caller
> > anyway.
>
> No we don't, I added it only to have symmetry with the
> vfio_group_put() naming.
>
> Alex, what is your taste here?
I like the symmetry, but afaict this use of inc_not_zero is
specifically to cover the gap where vfio_group_fops_open() could race
vfio_group_put, right? All the use cases of vfio_group_get() guarantee
that the group->users refcount is >0 based on either the fact that it's
found under the vfio.group_lock or the that call is based on an existing
open group.
If that's true, then this helper function seems like it invites
confusion and misuse more so than providing symmetry. Open coding with
a comment explaining the vfio_group_get() calling requirements and
noting the race this inc_not_zero usage solves seems like the better
option to me. Thanks,
Alex
