| Message ID | 583cde9c.3223ed0a.7f0c2.886e@mx.google.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: > From: Li Qiang <liqiang6-s@360.cn> > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated > in realize isn't freed thus leaking memory leak. This patch avoid > this through adding the exit function. I will just note that the real hardware is not hot-pluggable. However we don't need to stick to the real hardware capabilities, so that's OK. > Signed-off-by: Li Qiang <liqiang6-s@360.cn> > --- > hw/watchdog/wdt_i6300esb.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c > index a83d951..49b3cd1 100644 > --- a/hw/watchdog/wdt_i6300esb.c > +++ b/hw/watchdog/wdt_i6300esb.c > @@ -428,6 +428,14 @@ 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); > +} > + > static WatchdogTimerModel model = { > .wdt_name = "i6300esb", > .wdt_description = "Intel 6300ESB", > @@ -441,6 +449,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; The wdt_diag288.c file seems to use k->unrealize for this purpose. I don't know which is correct however. Rich. > k->vendor_id = PCI_VENDOR_ID_INTEL; > k->device_id = PCI_DEVICE_ID_INTEL_ESB_9; > k->class_id = PCI_CLASS_SYSTEM_OTHER; > -- > 1.8.3.1
Hi 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: > > From: Li Qiang <liqiang6-s@360.cn> > > > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated > > in realize isn't freed thus leaking memory leak. This patch avoid > > this through adding the exit function. > > I will just note that the real hardware is not hot-pluggable. However > we don't need to stick to the real hardware capabilities, so that's OK. > > If the hardware is not hot-pluggable, we can set dc->hotpluggable = false. > Signed-off-by: Li Qiang <liqiang6-s@360.cn> > > --- > > hw/watchdog/wdt_i6300esb.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c > > index a83d951..49b3cd1 100644 > > --- a/hw/watchdog/wdt_i6300esb.c > > +++ b/hw/watchdog/wdt_i6300esb.c > > @@ -428,6 +428,14 @@ 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); > > +} > > + > > static WatchdogTimerModel model = { > > .wdt_name = "i6300esb", > > .wdt_description = "Intel 6300ESB", > > @@ -441,6 +449,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; > > The wdt_diag288.c file seems to use k->unrealize for this purpose. > I don't know which is correct however. > > > wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for PCIDeviceClass. I have tested this patch, it's ok. So we should make it not hotpluggable or just remain this? Thanks Li Qiang > > > k->vendor_id = PCI_VENDOR_ID_INTEL; > > k->device_id = PCI_DEVICE_ID_INTEL_ESB_9; > > k->class_id = PCI_CLASS_SYSTEM_OTHER; > > -- > > 1.8.3.1 > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~ > rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-p2v converts physical machines to virtual machines. Boot with a > live CD or over the network (PXE) and turn machines into KVM guests. > http://libguestfs.org/virt-v2v >
On Tue, Nov 29, 2016 at 04:56:55PM +0800, Li Qiang wrote: > Hi > > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > > > On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: > > > From: Li Qiang <liqiang6-s@360.cn> > > > > > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated > > > in realize isn't freed thus leaking memory leak. This patch avoid > > > this through adding the exit function. > > > > I will just note that the real hardware is not hot-pluggable. However > > we don't need to stick to the real hardware capabilities, so that's OK. > > > > > > If the hardware is not hot-pluggable, we can set dc->hotpluggable = false. No no no! The hardware hasn't been made for a decade or two, we don't need to stick with what the real hardware did. Rich.
2016-11-29 17:00 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > On Tue, Nov 29, 2016 at 04:56:55PM +0800, Li Qiang wrote: > > Hi > > > > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > > > > > On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: > > > > From: Li Qiang <liqiang6-s@360.cn> > > > > > > > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated > > > > in realize isn't freed thus leaking memory leak. This patch avoid > > > > this through adding the exit function. > > > > > > I will just note that the real hardware is not hot-pluggable. However > > > we don't need to stick to the real hardware capabilities, so that's OK. > > > > > > > > > > If the hardware is not hot-pluggable, we can set dc->hotpluggable = > false. > > No no no! The hardware hasn't been made for a decade or two, > we don't need to stick with what the real hardware did. > > OK, then I think this patch is OK. > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~ > rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > libguestfs lets you edit virtual machines. Supports shell scripting, > bindings from many languages. http://libguestfs.org >
Li Qiang <liq3ea@gmail.com> writes: > Hi > > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > >> On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: >> > From: Li Qiang <liqiang6-s@360.cn> >> > >> > When the Intel 6300ESB watchdog is hot unplug. The timer allocated >> > in realize isn't freed thus leaking memory leak. This patch avoid >> > this through adding the exit function. >> >> I will just note that the real hardware is not hot-pluggable. However >> we don't need to stick to the real hardware capabilities, so that's OK. >> >> > > If the hardware is not hot-pluggable, we can set dc->hotpluggable = false. > > >> Signed-off-by: Li Qiang <liqiang6-s@360.cn> >> > --- >> > hw/watchdog/wdt_i6300esb.c | 9 +++++++++ >> > 1 file changed, 9 insertions(+) >> > >> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c >> > index a83d951..49b3cd1 100644 >> > --- a/hw/watchdog/wdt_i6300esb.c >> > +++ b/hw/watchdog/wdt_i6300esb.c >> > @@ -428,6 +428,14 @@ 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); >> > +} >> > + >> > static WatchdogTimerModel model = { >> > .wdt_name = "i6300esb", >> > .wdt_description = "Intel 6300ESB", >> > @@ -441,6 +449,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; >> >> The wdt_diag288.c file seems to use k->unrealize for this purpose. >> I don't know which is correct however. >> >> >> > wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for > PCIDeviceClass. Method exit() is deprecated, please use unrealize() in new code. [...]
2016-11-29 18:49 GMT+08:00 Markus Armbruster <armbru@redhat.com>: > Li Qiang <liq3ea@gmail.com> writes: > > > Hi > > > > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>: > > > >> On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote: > >> > From: Li Qiang <liqiang6-s@360.cn> > >> > > >> > When the Intel 6300ESB watchdog is hot unplug. The timer allocated > >> > in realize isn't freed thus leaking memory leak. This patch avoid > >> > this through adding the exit function. > >> > >> I will just note that the real hardware is not hot-pluggable. However > >> we don't need to stick to the real hardware capabilities, so that's OK. > >> > >> > > > > If the hardware is not hot-pluggable, we can set dc->hotpluggable = > false. > > > > > >> Signed-off-by: Li Qiang <liqiang6-s@360.cn> > >> > --- > >> > hw/watchdog/wdt_i6300esb.c | 9 +++++++++ > >> > 1 file changed, 9 insertions(+) > >> > > >> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c > >> > index a83d951..49b3cd1 100644 > >> > --- a/hw/watchdog/wdt_i6300esb.c > >> > +++ b/hw/watchdog/wdt_i6300esb.c > >> > @@ -428,6 +428,14 @@ 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); > >> > +} > >> > + > >> > static WatchdogTimerModel model = { > >> > .wdt_name = "i6300esb", > >> > .wdt_description = "Intel 6300ESB", > >> > @@ -441,6 +449,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; > >> > >> The wdt_diag288.c file seems to use k->unrealize for this purpose. > >> I don't know which is correct however. > >> > >> > >> > > wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for > > PCIDeviceClass. > > Method exit() is deprecated, please use unrealize() in new code. > > [...] > Hello, IIUC in the PCIDeviceClass definition, there is just an exit member not a unrealize. The DeviceClass has an unrealize member. Thanks.
Li Qiang <liq3ea@gmail.com> writes: > 2016-11-29 18:49 GMT+08:00 Markus Armbruster <armbru@redhat.com>: [...] >> Method exit() is deprecated, please use unrealize() in new code. >> >> [...] >> > > Hello, > > IIUC in the PCIDeviceClass definition, there is just an exit member not a > unrealize. > The DeviceClass has an unrealize member. You're right. A less confusing PCIDeviceClass would be nice.
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c index a83d951..49b3cd1 100644 --- a/hw/watchdog/wdt_i6300esb.c +++ b/hw/watchdog/wdt_i6300esb.c @@ -428,6 +428,14 @@ 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); +} + static WatchdogTimerModel model = { .wdt_name = "i6300esb", .wdt_description = "Intel 6300ESB", @@ -441,6 +449,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;