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

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Kirti Wankhede <kwankhede@nvidia.com>
Cc: "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 03:53:10 +0000	[thread overview]
Message-ID: <MWHPR11MB18864F0836984277A52BB4CB8C619@MWHPR11MB1886.namprd11.prod.outlook.com> (raw)
In-Reply-To: <10-v1-7dedf20b2b75+4f785-vfio2_jgg@nvidia.com>

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 24, 2021 1:55 AM
> 
> mdev_device->type->parent is the same thing.
> 
> The struct mdev_device was relying on the kref on the mdev_parent to also
> indirectly hold a kref on the mdev_type pointer. Now that the type holds a
> kref on the parent we can directly kref the mdev_type and remove this
> implicit relationship.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/mdev/mdev_core.c | 13 +++++--------
>  drivers/vfio/mdev/vfio_mdev.c | 14 +++++++-------
>  include/linux/mdev.h          |  1 -
>  3 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 4b5e372ed58f26..493df3da451339 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -29,7 +29,7 @@ static DEFINE_MUTEX(mdev_list_lock);
> 
>  struct device *mdev_parent_dev(struct mdev_device *mdev)
>  {
> -	return mdev->parent->dev;
> +	return mdev->type->parent->dev;
>  }
>  EXPORT_SYMBOL(mdev_parent_dev);
> 
> @@ -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.

>  	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.

> 
>  	mutex_lock(&mdev_list_lock);
>  	list_del(&mdev->next);
> @@ -250,9 +249,8 @@ int mdev_device_create(struct mdev_type *type,
> const guid_t *uuid)
>  	mdev->dev.release = mdev_device_release;
>  	mdev->dev.groups = parent->ops->mdev_attr_groups;
>  	mdev->type = type;
> -	mdev->parent = parent;
>  	/* Pairs with the put in mdev_device_release() */
> -	mdev_get_parent(parent);
> +	kobject_get(&type->kobj);
> 
>  	guid_copy(&mdev->uuid, uuid);
>  	list_add(&mdev->next, &mdev_list);
> @@ -300,7 +298,7 @@ int mdev_device_create(struct mdev_type *type,
> const guid_t *uuid)
>  int mdev_device_remove(struct mdev_device *mdev)
>  {
>  	struct mdev_device *tmp;
> -	struct mdev_parent *parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	mutex_lock(&mdev_list_lock);
>  	list_for_each_entry(tmp, &mdev_list, next) {
> @@ -321,7 +319,6 @@ int mdev_device_remove(struct mdev_device *mdev)
>  	mdev->active = false;
>  	mutex_unlock(&mdev_list_lock);
> 
> -	parent = mdev->parent;
>  	/* Check if parent unregistration has started */
>  	if (!down_read_trylock(&parent->unreg_sem))
>  		return -ENODEV;
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index cc9507ed85a181..922729071c5a8e 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -24,7 +24,7 @@
>  static int vfio_mdev_open(struct vfio_device *core_vdev)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	int ret;
> 
> @@ -44,7 +44,7 @@ static int vfio_mdev_open(struct vfio_device
> *core_vdev)
>  static void vfio_mdev_release(struct vfio_device *core_vdev)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (likely(parent->ops->release))
>  		parent->ops->release(mdev);
> @@ -56,7 +56,7 @@ static long vfio_mdev_unlocked_ioctl(struct vfio_device
> *core_vdev,
>  				     unsigned int cmd, unsigned long arg)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (unlikely(!parent->ops->ioctl))
>  		return -EINVAL;
> @@ -68,7 +68,7 @@ static ssize_t vfio_mdev_read(struct vfio_device
> *core_vdev, char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (unlikely(!parent->ops->read))
>  		return -EINVAL;
> @@ -81,7 +81,7 @@ static ssize_t vfio_mdev_write(struct vfio_device
> *core_vdev,
>  			       loff_t *ppos)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (unlikely(!parent->ops->write))
>  		return -EINVAL;
> @@ -93,7 +93,7 @@ static int vfio_mdev_mmap(struct vfio_device
> *core_vdev,
>  			  struct vm_area_struct *vma)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (unlikely(!parent->ops->mmap))
>  		return -EINVAL;
> @@ -104,7 +104,7 @@ static int vfio_mdev_mmap(struct vfio_device
> *core_vdev,
>  static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int
> count)
>  {
>  	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> -	struct mdev_parent *parent = mdev->parent;
> +	struct mdev_parent *parent = mdev->type->parent;
> 
>  	if (parent->ops->request)
>  		parent->ops->request(mdev, count);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 349e8ac1fe3382..fb582adda28a9b 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -14,7 +14,6 @@ struct mdev_type;
> 
>  struct mdev_device {
>  	struct device dev;
> -	struct mdev_parent *parent;
>  	guid_t uuid;
>  	void *driver_data;
>  	struct list_head next;
> --
> 2.31.0