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: Thu, 25 Apr 2013 21:22:04 +0300 Message-ID: <20130425182204.GH16740@redhat.com> References: <1364954273-18196-1-git-send-email-scottwood@freescale.com> <1365811727-24431-1-git-send-email-scottwood@freescale.com> <1365811727-24431-2-git-send-email-scottwood@freescale.com> <20130425094324.GY12401@redhat.com> <5A22B05F-C1C5-439F-8AC6-D16137A9D086@suse.de> <1366908668.30341.1@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: <1366908668.30341.1@snotra> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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? > If you make > 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? -- Gleb. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Date: Thu, 25 Apr 2013 18:22:04 +0000 Subject: Re: [PATCH v4 1/6] kvm: add device control API Message-Id: <20130425182204.GH16740@redhat.com> List-Id: References: <1364954273-18196-1-git-send-email-scottwood@freescale.com> <1365811727-24431-1-git-send-email-scottwood@freescale.com> <1365811727-24431-2-git-send-email-scottwood@freescale.com> <20130425094324.GY12401@redhat.com> <5A22B05F-C1C5-439F-8AC6-D16137A9D086@suse.de> <1366908668.30341.1@snotra> In-Reply-To: <1366908668.30341.1@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Scott Wood Cc: Alexander Graf , kvm-ppc@vger.kernel.org, kvm@vger.kernel.org, paulus@samba.org 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? > If you make > 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? -- Gleb.