From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v4 1/6] kvm: add device control API Date: Fri, 26 Apr 2013 12:53:18 +0300 Message-ID: <20130426095318.GJ16740@redhat.com> References: <20130425182204.GH16740@redhat.com> <1366916360.30341.7@snotra> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alexander Graf , kvm-ppc@vger.kernel.org, kvm@vger.kernel.org, paulus@samba.org To: Scott Wood Return-path: Content-Disposition: inline In-Reply-To: <1366916360.30341.7@snotra> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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? -- Gleb.