Re: [PATCH v4 1/6] kvm: add device control API

[Gleb Natapov <gleb@redhat.com>]

On Fri, Apr 26, 2013 at 11:55:27AM +0200, Alexander Graf wrote:
> 
> On 26.04.2013, at 11:53, Gleb Natapov wrote:
> 
> > On Thu, Apr 25, 2013 at 01:59:20PM -0500, Scott Wood wrote:
> >> On 04/25/2013 01:22:04 PM, Gleb Natapov wrote:
> >>> On Thu, Apr 25, 2013 at 11:51:08AM -0500, Scott Wood wrote:
> >>>> On 04/25/2013 05:47:39 AM, Alexander Graf wrote:
> >>>>> 
> >>>>> On 25.04.2013, at 11:43, Gleb Natapov wrote:
> >>>>> 
> >>>>>>> +void kvm_device_put(struct kvm_device *dev)
> >>>>>>> +{
> >>>>>>> +	if (atomic_dec_and_test(&dev->users))
> >>>>>>> +		dev->ops->destroy(dev);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int kvm_device_release(struct inode *inode, struct file
> >>>>> *filp)
> >>>>>>> +{
> >>>>>>> +	struct kvm_device *dev = filp->private_data;
> >>>>>>> +	struct kvm *kvm = dev->kvm;
> >>>>>>> +
> >>>>>>> +	kvm_device_put(dev);
> >>>>>>> +	kvm_put_kvm(kvm);
> >>>>>> We may put kvm only if users goes to zero, otherwise kvm can be
> >>>>>> freed while something holds a reference to a device. Why not make
> >>>>>> kvm_device_put() do it?
> >>>>> 
> >>>>> Nice catch. I'll change the patch so it does the kvm_put_kvm
> >>>>> inside kvm_device_put's destroy branch.
> >>>> 
> >>>> No, please don't.  The KVM reference being "put" here is associated
> >>>> with the file descriptor, not with the MPIC object.
> >>> Is it so? Device holds a pointer to kvm, so it increments kvm
> >>> reference
> >>> to make sure the pointer is valid. What prevents kvm from been
> >>> destroyed
> >>> while device is still in use in current code?
> >> 
> >> Where will that kvm pointer be used, after all the file descriptors
> >> go away and the vcpus stop running?  mmio_mapped guards against
> >> unmapping the MMIO if it's already been unmapped due to KVM
> >> destruction.  We don't have any timers or other delayed work.
> >> 
> > MPIC does not, but timer device will have one.
> > 
> >> Well, I do see one place, that Alex added -- the NULLing out of
> >> dev->kvm->arch.mpic, which didn't exist in my patchset.
> >> 
> >>>> that change I think you'll have circular references and thus a
> >>>> memory leak, because the vcpus can hold a reference to the MPIC
> >>>> object.
> >>>> 
> >>> How circular reference can be created?
> >> 
> >> MPIC holds reference on KVM, vcpu holds reference on MPIC, and vcpu
> >> is not destroyed until KVM is destroyed.
> >> 
> > Yes, you are right. So we need to think about how to fix it in a
> > different way. What about holding all devices in kvm->devices[] array
> > and destroy them during kvm destruction, like we do for vcpus?
> 
> You should really look at your patches in LIFO order :). A patch doing that was already sent by Scott last night and is in v4 of my patch set.
> 
> 
I tried! This causes starvation for some patches. I need better algorithm :)

--
			Gleb.