From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkB0N-0000GM-ST for qemu-devel@nongnu.org; Wed, 05 Jun 2013 06:31:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkB0H-00004q-7b for qemu-devel@nongnu.org; Wed, 05 Jun 2013 06:31:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkB0G-0008WK-Vr for qemu-devel@nongnu.org; Wed, 05 Jun 2013 06:31:41 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r55AVdwM014828 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 5 Jun 2013 06:31:40 -0400 Date: Wed, 5 Jun 2013 13:32:14 +0300 From: "Michael S. Tsirkin" Message-ID: <20130605103214.GC31830@redhat.com> References: <1370371954-8479-1-git-send-email-pbonzini@redhat.com> <1370371954-8479-12-git-send-email-pbonzini@redhat.com> <20130604220328.GB30400@redhat.com> <51AE6CC0.4050808@redhat.com> <20130605045348.GA31253@redhat.com> <51AEED43.2080302@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51AEED43.2080302@redhat.com> Subject: Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote: > Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: > > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: > >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: > >>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { > >>>>> + msix_free(dev); > >>>>> + } > >>>>> + > >>>>> dev->msix_table = g_malloc0(table_size); > >>>>> dev->msix_pba = g_malloc0(pba_size); > >>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); > >>> Wow msix_init calls msix_free, and not on error path? > >>> What's going on here? > >> > >> I wasn't too sure that you could get here only with NULL > >> msix_table/pba/entry_used and wanted to protect against leaks. I'll > >> change it to an assertion. > > > > I don't think we should require users allocate all memory with g_malloc0. > > So no assertion either. > > Assertion that is is NULL, followed by g_malloc0? No because who sets it to NULL the first time? msix_init just started. > > If there's a leak there was always a leak > > No, there wasn't because msix_uninit would have freed the memory. That is, > > msix_init > msix_uninit > msix_init > msix_uninit > > had no leak. Instead, now msix_free is going to be called just once, > right before freeing the object itself: > > msix_init > msix_uninit > msix_init *** > msix_uninit > msix_free > > and will have a leak at ***. Yes. And this looks completely sane from outside, so this is a bad API. The way to fix it is not with asserts in code, we need a good API: alloc/free init/uninit ... The problem apparently starts in generic code, let's fix it there? > I don't think this can happen, unrealize > should never be followed by another realize right now, This is not an msix specific problem, I don't think msix should debug generic core - this will just lead to proliferation of asserts. This really should be documented prominently in generic code. Also how about some asserts in generic code making sure ordering is sane? > but perhaps in > the future it will be if we implement something like "device_poweroff" > and "device_poweron". > > Paolo > , let's focus on the > > API change in this series, OK? > > > >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) > >>>>> msix_free_irq_entries(dev); > >>>>> dev->msix_entries_nr = 0; > >>>>> memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); > >>>>> - memory_region_destroy(&dev->msix_pba_mmio); > >>>>> - g_free(dev->msix_pba); > >>>>> - dev->msix_pba = NULL; > >>>>> memory_region_del_subregion(table_bar, &dev->msix_table_mmio); > >>>>> - memory_region_destroy(&dev->msix_table_mmio); > >>>>> - g_free(dev->msix_table); > >>>>> + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >>>>> +} > >>>>> + > >>>>> +void msix_free(PCIDevice *dev) > >>>>> +{ > >>>>> + if (dev->msix_pba) { > >>>>> + memory_region_destroy(&dev->msix_pba_mmio); > >>>>> + g_free(dev->msix_pba); > >>>>> + } > >>>>> + dev->msix_pba = NULL; > >>>>> + > >>>>> + if (dev->msix_table) { > >>>>> + memory_region_destroy(&dev->msix_table_mmio); > >>>>> + g_free(dev->msix_table); > >>>>> + } > >>>>> dev->msix_table = NULL; > >>>>> + > >>>>> g_free(dev->msix_entry_used); > >>>>> dev->msix_entry_used = NULL; > >>>>> - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >>>>> } > >>>>> > >>>>> void msix_uninit_exclusive_bar(PCIDevice *dev) > >>> As long as we had init and uninit, it was mostly > >>> self-documenting. > >>> Now, there are two cleanup functions, so please add documentation. > >> > >> Yes, will do. > >> > >> Paolo > > > >