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. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html