* [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize
@ 2016-07-22 19:50 minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 1/4] ipmi_bmc_sim: Remove an unnecessary mutex minyard
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: minyard @ 2016-07-22 19:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, minyard
Changes from v2:
Set the timers to NULL when unrealizing them, as the data structure
is still intact.
Split out the removal of the mutex from ipmi_bmc_sim to its own patch.
I did figure out a way to test these. You can set the realize bool
in the qapi for the device to cause it to be realized/unrealized.
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.
I have a separate patch to make the BMCs not hot-pluggable and
fix up a few other small things I saw as a result of understanding
this better. But those probably don't belong here.
Thanks,
-corey
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] ipmi_bmc_sim: Remove an unnecessary mutex
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
@ 2016-07-22 19:50 ` minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 2/4] wdt_i6300esb: Free timer minyard
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: minyard @ 2016-07-22 19:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Get rid of the unnecessary mutex, it was a vestige
of something else that was not done. That way we don't
have to free it.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/ipmi/ipmi_bmc_sim.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index dc9c14c..b742d8d 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -217,7 +217,6 @@ struct IPMIBmcSim {
/* Odd netfns are for responses, so we only need the even ones. */
const IPMINetfn *netfns[MAX_NETFNS / 2];
- QemuMutex lock;
/* We allow one event in the buffer */
uint8_t evtbuf[16];
@@ -940,7 +939,6 @@ static void get_msg(IPMIBmcSim *ibs,
{
IPMIRcvBufEntry *msg;
- qemu_mutex_lock(&ibs->lock);
if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
rsp_buffer_set_error(rsp, 0x80); /* Queue empty */
goto out;
@@ -960,7 +958,6 @@ static void get_msg(IPMIBmcSim *ibs,
}
out:
- qemu_mutex_unlock(&ibs->lock);
return;
}
@@ -1055,11 +1052,9 @@ static void send_msg(IPMIBmcSim *ibs,
end_msg:
msg->buf[msg->len] = ipmb_checksum(msg->buf, msg->len, 0);
msg->len++;
- qemu_mutex_lock(&ibs->lock);
QTAILQ_INSERT_TAIL(&ibs->rcvbufs, msg, entry);
ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE;
k->set_atn(s, 1, attn_irq_enabled(ibs));
- qemu_mutex_unlock(&ibs->lock);
}
static void do_watchdog_reset(IPMIBmcSim *ibs)
@@ -1753,7 +1748,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
unsigned int i;
IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
- qemu_mutex_init(&ibs->lock);
QTAILQ_INIT(&ibs->rcvbufs);
ibs->bmc_global_enables = (1 << IPMI_BMC_EVENT_LOG_BIT);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] wdt_i6300esb: Free timer
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 1/4] ipmi_bmc_sim: Remove an unnecessary mutex minyard
@ 2016-07-22 19:50 ` minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 3/4] wdt_ib700: " minyard
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: minyard @ 2016-07-22 19:50 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, minyard, Corey Minyard, Richard W . M . Jones
From: Corey Minyard <cminyard@mvista.com>
Add an exit function to free the timer allocated in the
realize function.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
---
hw/watchdog/wdt_i6300esb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index a83d951..aa64457 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -428,6 +428,15 @@ static void i6300esb_realize(PCIDevice *dev, Error **errp)
/* qemu_register_coalesced_mmio (addr, 0x10); ? */
}
+static void i6300esb_exit(PCIDevice *dev)
+{
+ I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
+
+ timer_del(d->timer);
+ timer_free(d->timer);
+ d->timer = NULL;
+}
+
static WatchdogTimerModel model = {
.wdt_name = "i6300esb",
.wdt_description = "Intel 6300ESB",
@@ -441,6 +450,7 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
k->config_read = i6300esb_config_read;
k->config_write = i6300esb_config_write;
k->realize = i6300esb_realize;
+ k->exit = i6300esb_exit;
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
k->class_id = PCI_CLASS_SYSTEM_OTHER;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] wdt_ib700: Free timer
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 1/4] ipmi_bmc_sim: Remove an unnecessary mutex minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 2/4] wdt_i6300esb: Free timer minyard
@ 2016-07-22 19:50 ` minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 4/4] ipmi_bmc_sim: Add a proper unrealize function minyard
2016-07-23 7:46 ` [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize Paolo Bonzini
4 siblings, 0 replies; 9+ messages in thread
From: minyard @ 2016-07-22 19:50 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, minyard, Corey Minyard, Richard W . M . Jones
From: Corey Minyard <cminyard@mvista.com>
Add an unrealize function to free the timer allocated in the
realize function and to delete the port memory added there,
too.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Richard W.M. Jones <rjones@redhat.com>
Cc: Marc-André Lureau <mlureau@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
---
hw/watchdog/wdt_ib700.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index 532afe8..6d5fc4c 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -117,6 +117,17 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
}
+static void wdt_ib700_unrealize(DeviceState *dev, Error **errp)
+{
+ IB700State *s = IB700(dev);
+
+ timer_del(s->timer);
+ timer_free(s->timer);
+ s->timer = NULL;
+ portio_list_del(&s->port_list);
+ portio_list_destroy(&s->port_list);
+}
+
static void wdt_ib700_reset(DeviceState *dev)
{
IB700State *s = IB700(dev);
@@ -136,6 +147,7 @@ static void wdt_ib700_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = wdt_ib700_realize;
+ dc->unrealize = wdt_ib700_unrealize;
dc->reset = wdt_ib700_reset;
dc->vmsd = &vmstate_ib700;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] ipmi_bmc_sim: Add a proper unrealize function
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
` (2 preceding siblings ...)
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 3/4] wdt_ib700: " minyard
@ 2016-07-22 19:50 ` minyard
2016-07-23 7:46 ` [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize Paolo Bonzini
4 siblings, 0 replies; 9+ messages in thread
From: minyard @ 2016-07-22 19:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Add an unrealize function to free the timer allocated in the
realize function, unregister the vmstate, and free any
pending messages.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/ipmi/ipmi_bmc_sim.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index b742d8d..fe92b93 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1780,12 +1780,28 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
}
+static void ipmi_sim_unrealize(DeviceState *dev, Error **errp)
+{
+ IPMIBmc *b = IPMI_BMC(dev);
+ IPMIRcvBufEntry *msg, *tmp;
+ IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
+
+ vmstate_unregister(NULL, &vmstate_ipmi_sim, ibs);
+ timer_del(ibs->timer);
+ timer_free(ibs->timer);
+ QTAILQ_FOREACH_SAFE(msg, &ibs->rcvbufs, entry, tmp) {
+ QTAILQ_REMOVE(&ibs->rcvbufs, msg, entry);
+ g_free(msg);
+ }
+}
+
static void ipmi_sim_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
dc->realize = ipmi_sim_realize;
+ dc->unrealize = ipmi_sim_unrealize;
bk->handle_command = ipmi_sim_handle_command;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
` (3 preceding siblings ...)
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 4/4] ipmi_bmc_sim: Add a proper unrealize function minyard
@ 2016-07-23 7:46 ` Paolo Bonzini
2016-07-23 13:18 ` Corey Minyard
4 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2016-07-23 7:46 UTC (permalink / raw)
To: minyard, qemu-devel; +Cc: Marc-André Lureau
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize
2016-07-23 7:46 ` [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize Paolo Bonzini
@ 2016-07-23 13:18 ` Corey Minyard
2016-07-23 15:16 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Corey Minyard @ 2016-07-23 13:18 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Marc-André Lureau
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.
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?
-corey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize
2016-07-23 13:18 ` Corey Minyard
@ 2016-07-23 15:16 ` Paolo Bonzini
2016-07-23 18:04 ` Corey Minyard
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2016-07-23 15:16 UTC (permalink / raw)
To: minyard, qemu-devel; +Cc: Marc-André Lureau
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.
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize
2016-07-23 15:16 ` Paolo Bonzini
@ 2016-07-23 18:04 ` Corey Minyard
0 siblings, 0 replies; 9+ messages in thread
From: Corey Minyard @ 2016-07-23 18:04 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Marc-André 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-23 18:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-22 19:50 [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 1/4] ipmi_bmc_sim: Remove an unnecessary mutex minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 2/4] wdt_i6300esb: Free timer minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 3/4] wdt_ib700: " minyard
2016-07-22 19:50 ` [Qemu-devel] [PATCH v3 4/4] ipmi_bmc_sim: Add a proper unrealize function minyard
2016-07-23 7:46 ` [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize Paolo Bonzini
2016-07-23 13:18 ` Corey Minyard
2016-07-23 15:16 ` Paolo Bonzini
2016-07-23 18:04 ` Corey Minyard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.