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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux