| Message ID | 7-v1-11d8272dc65a+4bd-vfio_ioctl_split_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Break up ioctl dispatch functions to one function per ioctl | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, August 18, 2022 12:07 AM > > Make it clear that this is the body of the ioctl - keep the mutex outside > the function since this function doesn't have and wouldn't benefit from > error unwind. but doing so make unset_container() unpair with set_container() and be the only one doing additional things in main ioctl body. I'd prefer to moving mutex inside unset_container() for better readability. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index f7b02d3fd3108b..78957f45c37a34 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -968,7 +968,7 @@ static void __vfio_group_unset_container(struct > vfio_group *group) > * the group, we know that still exists, therefore the only valid > * transition here is 1->0. > */ > -static int vfio_group_unset_container(struct vfio_group *group) > +static int vfio_group_ioctl_unset_container(struct vfio_group *group) > { > lockdep_assert_held_write(&group->group_rwsem); > > @@ -1271,7 +1271,7 @@ static long vfio_group_fops_unl_ioctl(struct file > *filep, > return vfio_group_ioctl_set_container(group, uarg); > case VFIO_GROUP_UNSET_CONTAINER: > down_write(&group->group_rwsem); > - ret = vfio_group_unset_container(group); > + ret = vfio_group_ioctl_unset_container(group); > up_write(&group->group_rwsem); > break; > } > -- > 2.37.2
On Mon, Aug 29, 2022 at 12:41:30AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Thursday, August 18, 2022 12:07 AM > > > > Make it clear that this is the body of the ioctl - keep the mutex outside > > the function since this function doesn't have and wouldn't benefit from > > error unwind. > > but doing so make unset_container() unpair with set_container() and > be the only one doing additional things in main ioctl body. > > I'd prefer to moving mutex inside unset_container() for better readability. Yes, I tried both ways and ended up here since adding the goto unwind was kind of ungainly for this function. Don't mind either way The functions are not intended as strict pairs, they are ioctl dispatch functions. Jason
On Mon, 29 Aug 2022 12:28:07 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Mon, Aug 29, 2022 at 12:41:30AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Thursday, August 18, 2022 12:07 AM > > > > > > Make it clear that this is the body of the ioctl - keep the mutex outside > > > the function since this function doesn't have and wouldn't benefit from > > > error unwind. > > > > but doing so make unset_container() unpair with set_container() and > > be the only one doing additional things in main ioctl body. > > > > I'd prefer to moving mutex inside unset_container() for better readability. > > Yes, I tried both ways and ended up here since adding the goto unwind > was kind of ungainly for this function. Don't mind either way > > The functions are not intended as strict pairs, they are ioctl > dispatch functions. The lockdep annotation seems sufficient, but what about simply prefixing the unset ioctl function with underscores to reinforce the locking requirement, as done by the called function __vfio_group_unset_container()? Thanks, Alex
On Tue, Aug 30, 2022 at 10:42:31AM -0600, Alex Williamson wrote: > On Mon, 29 Aug 2022 12:28:07 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Mon, Aug 29, 2022 at 12:41:30AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Thursday, August 18, 2022 12:07 AM > > > > > > > > Make it clear that this is the body of the ioctl - keep the mutex outside > > > > the function since this function doesn't have and wouldn't benefit from > > > > error unwind. > > > > > > but doing so make unset_container() unpair with set_container() and > > > be the only one doing additional things in main ioctl body. > > > > > > I'd prefer to moving mutex inside unset_container() for better readability. > > > > Yes, I tried both ways and ended up here since adding the goto unwind > > was kind of ungainly for this function. Don't mind either way > > > > The functions are not intended as strict pairs, they are ioctl > > dispatch functions. > > The lockdep annotation seems sufficient, but what about simply > prefixing the unset ioctl function with underscores to reinforce the > locking requirement, as done by the called function > __vfio_group_unset_container()? Thanks, Could do, but IMHO, that is not a popular convention in VFIO (anymore?). I've been adding lockdep annotations, not adding __ to indicate the function is called with some kind of lock. In my tree __vfio_group_get_from_iommu() is the only one left using that style. uset_container was turned into vfio_container_detatch_group() in aother series Conversly we have __vfio_register_dev() which I guess __ indicates is internal or wrappered, not some statement about locking. I think this has become the more popular usage of the __ generally in the kernel So I will do a v2 with the extra goto unwind then Are there any other remarks before I do it? Thanks, Jason
On Tue, 30 Aug 2022 13:51:18 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Tue, Aug 30, 2022 at 10:42:31AM -0600, Alex Williamson wrote: > > On Mon, 29 Aug 2022 12:28:07 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > On Mon, Aug 29, 2022 at 12:41:30AM +0000, Tian, Kevin wrote: > > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > > Sent: Thursday, August 18, 2022 12:07 AM > > > > > > > > > > Make it clear that this is the body of the ioctl - keep the mutex outside > > > > > the function since this function doesn't have and wouldn't benefit from > > > > > error unwind. > > > > > > > > but doing so make unset_container() unpair with set_container() and > > > > be the only one doing additional things in main ioctl body. > > > > > > > > I'd prefer to moving mutex inside unset_container() for better readability. > > > > > > Yes, I tried both ways and ended up here since adding the goto unwind > > > was kind of ungainly for this function. Don't mind either way > > > > > > The functions are not intended as strict pairs, they are ioctl > > > dispatch functions. > > > > The lockdep annotation seems sufficient, but what about simply > > prefixing the unset ioctl function with underscores to reinforce the > > locking requirement, as done by the called function > > __vfio_group_unset_container()? Thanks, > > Could do, but IMHO, that is not a popular convention in VFIO > (anymore?). > > I've been adding lockdep annotations, not adding __ to indicate > the function is called with some kind of lock. > > In my tree __vfio_group_get_from_iommu() is the only one left using > that style. uset_container was turned into > vfio_container_detatch_group() in aother series > > Conversly we have __vfio_register_dev() which I guess __ indicates is > internal or wrappered, not some statement about locking. I think this > has become the more popular usage of the __ generally in the kernel > > So I will do a v2 with the extra goto unwind then > > Are there any other remarks before I do it? None from me. The pile of things relying on this series is growing, so it looks like time to push v2. Thanks, Alex
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index f7b02d3fd3108b..78957f45c37a34 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -968,7 +968,7 @@ static void __vfio_group_unset_container(struct vfio_group *group) * the group, we know that still exists, therefore the only valid * transition here is 1->0. */ -static int vfio_group_unset_container(struct vfio_group *group) +static int vfio_group_ioctl_unset_container(struct vfio_group *group) { lockdep_assert_held_write(&group->group_rwsem); @@ -1271,7 +1271,7 @@ static long vfio_group_fops_unl_ioctl(struct file *filep, return vfio_group_ioctl_set_container(group, uarg); case VFIO_GROUP_UNSET_CONTAINER: down_write(&group->group_rwsem); - ret = vfio_group_unset_container(group); + ret = vfio_group_ioctl_unset_container(group); up_write(&group->group_rwsem); break; }
Make it clear that this is the body of the ioctl - keep the mutex outside the function since this function doesn't have and wouldn't benefit from error unwind. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)