Re: [PATCH 10/18] vfio/mdev: Remove duplicate storage of parent in mdev_device

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Kirti Wankhede <kwankhede@nvidia.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH 10/18] vfio/mdev: Remove duplicate storage of parent in mdev_device
Date: Fri, 26 Mar 2021 08:53:50 -0300	[thread overview]
Message-ID: <20210326115350.GM2356281@nvidia.com> (raw)
In-Reply-To: <MWHPR11MB18864F0836984277A52BB4CB8C619@MWHPR11MB1886.namprd11.prod.outlook.com>

On Fri, Mar 26, 2021 at 03:53:10AM +0000, Tian, Kevin wrote:

> > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref)
> >  /* Caller must hold parent unreg_sem read or write lock */
> >  static void mdev_device_remove_common(struct mdev_device *mdev)
> >  {
> > -	struct mdev_parent *parent;
> > +	struct mdev_parent *parent = mdev->type->parent;
> 
> What about having a wrapper here, like mdev_parent_dev? For
> readability it's not necessary to show that the parent is indirectly
> retrieved through mdev_type.

I think that is too much wrappering, we only have three usages of the
mdev->type->parent sequence and two are already single line inlines.

> >  	int ret;
> > 
> >  	mdev_remove_sysfs_files(mdev);
> >  	device_del(&mdev->dev);
> > -	parent = mdev->parent;
> >  	lockdep_assert_held(&parent->unreg_sem);
> >  	ret = parent->ops->remove(mdev);
> >  	if (ret)
> > @@ -212,7 +211,7 @@ static void mdev_device_release(struct device *dev)
> >  	struct mdev_device *mdev = to_mdev_device(dev);
> > 
> >  	/* Pairs with the get in mdev_device_create() */
> > -	mdev_put_parent(mdev->parent);
> > +	kobject_put(&mdev->type->kobj);
> 
> Maybe keep mdev_get/put_parent and change them to accept "struct
> mdev_device *" parameter like other places.

We do keep mdev_get/put_parent() they manipulate the refcount inside
the parent and this is only done in side the type logic now.

This is get/put of the refcount on the type, and this is the only
place that does it, so I don't see a reason for more wrappers.

Jason