Re: [PATCH 05/13] kvm/vfio: Provide struct kvm_device_ops::release() insted of ::destroy()

From: Alex Williamson <alex.williamson@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Yi Liu <yi.l.liu@intel.com>,
	kevin.tian@intel.com, cohuck@redhat.com, eric.auger@redhat.com,
	nicolinc@nvidia.com, kvm@vger.kernel.org, mjrosato@linux.ibm.com,
	chao.p.peng@linux.intel.com, yi.y.sun@linux.intel.com,
	peterx@redhat.com, jasowang@redhat.com,
	suravee.suthikulpanit@amd.com
Subject: Re: [PATCH 05/13] kvm/vfio: Provide struct kvm_device_ops::release() insted of ::destroy()
Date: Thu, 19 Jan 2023 13:04:38 -0700	[thread overview]
Message-ID: <20230119130438.25387127.alex.williamson@redhat.com> (raw)
In-Reply-To: <Y8mU1R0lPl1T5koj@nvidia.com>

On Thu, 19 Jan 2023 15:07:01 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, Jan 17, 2023 at 05:49:34AM -0800, Yi Liu wrote:
> > This is to avoid a circular refcount problem between the kvm struct and
> > the device file. KVM modules holds device/group file reference when the
> > device/group is added and releases it per removal or the last kvm reference
> > is released. This reference model is ok for the group since there is no
> > kvm reference in the group paths.
> > 
> > But it is a problem for device file since the vfio devices may get kvm
> > reference in the device open path and put it in the device file release.
> > e.g. Intel kvmgt. This would result in a circular issue since the kvm
> > side won't put the device file reference if kvm reference is not 0, while
> > the vfio device side needs to put kvm reference in the release callback.
> > 
> > To solve this problem for device file, let vfio provide release() which
> > would be called once kvm file is closed, it won't depend on the last kvm
> > reference. Hence avoid circular refcount problem.
> > 
> > Suggested-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  virt/kvm/vfio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)  
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> From Alex's remarks please revise the commit message and add a Fixes
> line of some kind that this solves the deadlock Matthew was working
> on, and send it stand alone right away

Also revise the commit log since we'll be taking a reference in the
group model as well.  The function and comments should also be updated
s/destroy/release/.  Thanks,

Alex