From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50273) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bR1IN-0000ej-Rx for qemu-devel@nongnu.org; Sat, 23 Jul 2016 14:05:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bR1IJ-0003zc-LK for qemu-devel@nongnu.org; Sat, 23 Jul 2016 14:05:02 -0400 Received: from mail-pa0-x22f.google.com ([2607:f8b0:400e:c03::22f]:36423) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bR1IJ-0003zW-EE for qemu-devel@nongnu.org; Sat, 23 Jul 2016 14:04:59 -0400 Received: by mail-pa0-x22f.google.com with SMTP id pp5so48692953pac.3 for ; Sat, 23 Jul 2016 11:04:59 -0700 (PDT) Sender: Corey Minyard Reply-To: minyard@acm.org References: <1469217041-15358-1-git-send-email-minyard@acm.org> <57936EAA.7010705@acm.org> <8ff708c8-65f7-0272-3f55-f6507af7ae7d@redhat.com> From: Corey Minyard Message-ID: <5793B1C7.4020106@acm.org> Date: Sat, 23 Jul 2016 13:04:55 -0500 MIME-Version: 1.0 In-Reply-To: <8ff708c8-65f7-0272-3f55-f6507af7ae7d@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= On 07/23/2016 10:16 AM, Paolo Bonzini wrote: > > On 23/07/2016 15:18, Corey Minyard wrote: >> On 07/23/2016 02:46 AM, Paolo Bonzini wrote: >>> On 22/07/2016 21:50, minyard@acm.org wrote: >>>> This has kind of opened a can of worms for me, though. Looking >>>> at a lot of the devices, there is no unrealize function and that >>>> can leave a lot of things hanging. And for ISA bus devices, there >>>> is no way to unregister ports. >>> Right, this is because they aren't hotpluggable. >>> >>> I should dig out the huge patchset I had to make timers statically >>> allocated... >>> >>> Paolo >> Am I correct in saying, then, that instead of adding a finalize >> function to the IPMI BMC, we should instead make it not hot >> pluggable? And then the rest of my patches are not really >> relevant. I already have a function to set hotpluggable to >> false for the BMCs, I can post that. > If they are ISA devices they should already not be hot-unpluggable, > because none of the ISA bridges implements HotplugHandler. Because > that's just the way the bus works, it shouldn't be an issue. It's not exactly an ISA device. This is a BMC that an ISA device hooks to, but it's a separate device. >> From what I have seen, you can unrealize devices using the >> API, even if they are not hot pluggable, by setting the realized >> bool. Is that ok? > It's not great, but it's not a big deal either. > > The original idea behind "realize" was to have it as a sort of Vcc pin > where a false/true pulse would work as a reset, but this never > materialized. Now the true->false transition on realize is really only > used as part of a full guest-triggered hot-unplug sequence, which is > guest->hotplug_handler_unplug->(method call)->object_unparent. > > Because all HotplugHandlers call object_unparent, which in turn ends up > freeing the object, a false->true->false transition on realized (and > thus the timer leak) is not guest-triggerable. > > There are various fixes, including: > > - making the device non-hotpluggable > > - moving the timer_new and timer_free respectively to instance_init and > instance_finalize > > - making the timer static, which requires some small changes in the > timer API. Most of the last bullet is scriptable with Coccinelle. > > Right now I'd just do #2 or don't bother. > > Paolo I think I'm going to opt for #1, because the device isn't hot pluggable and if you try to unplug it qemu will crash. -corey