Re: [PATCH 2/8] vfio: Rename __vfio_group_unset_container()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	Cornelia Huck <cohuck@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH 2/8] vfio: Rename __vfio_group_unset_container()
Date: Fri, 2 Sep 2022 13:24:44 -0300	[thread overview]
Message-ID: <YxIuTF0qfy8c3cDj@nvidia.com> (raw)
In-Reply-To: <20220902083934.7e6f6438.alex.williamson@redhat.com>

On Fri, Sep 02, 2022 at 08:39:34AM -0600, Alex Williamson wrote:
> On Fri, 2 Sep 2022 03:51:01 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Friday, September 2, 2022 8:29 AM
> > > 
> > > On Wed, Aug 31, 2022 at 08:46:30AM +0000, Tian, Kevin wrote:  
> > > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > > Sent: Wednesday, August 31, 2022 9:02 AM
> > > > >
> > > > > To vfio_container_detatch_group(). This function is really a container
> > > > > function.
> > > > >
> > > > > Fold the WARN_ON() into it as a precondition assertion.
> > > > >
> > > > > A following patch will move the vfio_container functions to their own .c
> > > > > file.
> > > > >
> > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > > ---
> > > > >  drivers/vfio/vfio_main.c | 11 +++++------
> > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > > > index bfa6119ba47337..e145c87f208f3a 100644
> > > > > --- a/drivers/vfio/vfio_main.c
> > > > > +++ b/drivers/vfio/vfio_main.c
> > > > > @@ -928,12 +928,13 @@ static const struct file_operations vfio_fops = {
> > > > >  /*
> > > > >   * VFIO Group fd, /dev/vfio/$GROUP
> > > > >   */
> > > > > -static void __vfio_group_unset_container(struct vfio_group *group)
> > > > > +static void vfio_container_detatch_group(struct vfio_group *group)  
> > > >
> > > > s/detatch/detach/  
> > > 
> > > Oops
> > >   
> > > > Given it's a vfio_container function is it better to have a container pointer
> > > > as the first parameter, i.e.:
> > > >
> > > > static void vfio_container_detatch_group(struct vfio_container *container,
> > > > 		struct vfio_group *group)  
> > > 
> > > Ah, I'm not so sure, it seems weird to make the caller do
> > > group->container then pass the group in as well.
> > > 
> > > This call assumes the container is the container of the group, it
> > > doesn't work in situations where that isn't true.  
> > 
> > Yes, this is a valid interpretation on doing this way.
> > 
> > Other places e.g. iommu_detach_group(domain, group) go the other way
> > even if internally domain is not used at all. I kind of leave that pattern
> > in mind thus raised this comment. But not a strong opinion.
> > 
> > > 
> > > It is kind of weird layering in a way, arguably we should have the
> > > current group stored in the container if we want things to work that
> > > way, but that is a big change and not that wortwhile I think.
> > > 
> > > Patch 7 is pretty much the same, it doesn't work right unless the
> > > container and device are already matched
> > >   
> > 
> > If Alex won't have a different preference and with the typo fixed,
> 
> I don't get it, if a group is detaching itself from a container, why
> isn't it vfio_group_detach_container().  Given our naming scheme,
> vfio_container_detach_group() absolutely implies the args Kevin
> suggests, even if they're redundant.  vfio_foo_* functions should
> operate on a a vfio_foo object.

Look at the overall picture. This series moves struct vfio_container
into a .c file and then pulls all of the code that relies on it into
the c file too.

With our current function layout that results in these cut points:

struct vfio_container *vfio_container_from_file(struct file *filep);
int vfio_container_use(struct vfio_group *group);
void vfio_container_unuse(struct vfio_group *group);
int vfio_container_attach_group(struct vfio_container *container,
				struct vfio_group *group);
void vfio_container_detach_group(struct vfio_group *group);
void vfio_container_register_device(struct vfio_device *device);
void vfio_container_unregister_device(struct vfio_device *device);
long vfio_container_ioctl_check_extension(struct vfio_container *container,
					  unsigned long arg);
int vfio_container_pin_pages(struct vfio_device *device, dma_addr_t iova,
			     int npage, int prot, struct page **pages);
void vfio_container_unpin_pages(struct vfio_device *device, dma_addr_t iova,
				int npage);
int vfio_container_dma_rw(struct vfio_device *device, dma_addr_t iova,
			  void *data, size_t len, bool write);

Notice almost none of them use container as a function argument. The
container is always implied by another object that is linked to the
container.

Yet these are undeniably all container functions because they are only
necessary when the container code is actually being used. Every one of
this functions is bypassed if iommmufd is used. I even have a patch
that just replaces them all with nops if container and type 1 is
compiled out.

So, this presents two possiblities for naming. Either we call
everything in container.c vfio_container because it is primarily
related to the container.c *system*, or we label each function
according to OOP via the first argument type (vfio_XX_YY_container
perhaps) and still place them in container.c.

Keep in mind I have plans to see a group.c and rename vfio_main.c to
device.c, so having a vfio_group or vfio_device function in
container.c seems to get confusing.

In other words, I prefer to think of the group of functions above as
the exported API of the vfio container system (ie container.c) - not in
terms of an OOP modeling of a vfio_container object.

Jason