| Message ID | 1343187070-27371-5-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote: > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > > iohandler/bh/timer may use DeviceState when its refcnt=0, > postpone the reclaimer till they have done with it. > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > --- > include/qemu/object.h | 2 +- > main-loop.c | 4 ++++ > main-loop.h | 2 ++ > qemu-tool.c | 4 ++++ > qom/Makefile.objs | 2 +- > qom/object.c | 7 ++++++- > qom/reclaimer.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 59 insertions(+), 3 deletions(-) > create mode 100644 qom/reclaimer.c > > diff --git a/include/qemu/object.h b/include/qemu/object.h > index 8b17776..b233ee4 100644 > --- a/include/qemu/object.h > +++ b/include/qemu/object.h > @@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque), > */ > Object *container_get(Object *root, const char *path); > > - > +void qemu_reclaimer_enqueue(Object *obj); > #endif > diff --git a/main-loop.c b/main-loop.c > index eb3b6e6..f9cecc5 100644 > --- a/main-loop.c > +++ b/main-loop.c > @@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking) > them. */ > qemu_bh_poll(); > > + /* ref to device from iohandler/bh/timer do not obey the rules, so delay > + * reclaiming until now. > + */ > + qemu_device_reclaimer(); > return ret; > } > diff --git a/main-loop.h b/main-loop.h > index cedddf5..1a59a6d 100644 > --- a/main-loop.h > +++ b/main-loop.h > @@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh); > int qemu_bh_poll(void); > void qemu_bh_update_timeout(uint32_t *timeout); > > +void qemu_device_reclaimer(void); > + > #endif > diff --git a/qemu-tool.c b/qemu-tool.c > index 318c5fc..34d959b 100644 > --- a/qemu-tool.c > +++ b/qemu-tool.c > @@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void) > { > } > > +void qemu_device_reclaimer(void) > +{ > +} > + > int use_icount; > > void qemu_clock_warp(QEMUClock *clock) > diff --git a/qom/Makefile.objs b/qom/Makefile.objs > index 5ef060a..a579261 100644 > --- a/qom/Makefile.objs > +++ b/qom/Makefile.objs > @@ -1,4 +1,4 @@ > -qom-obj-y = object.o container.o qom-qobject.o > +qom-obj-y = object.o container.o qom-qobject.o reclaimer.o > qom-obj-twice-y = cpu.o > common-obj-y = $(qom-obj-twice-y) > user-obj-y = $(qom-obj-twice-y) > diff --git a/qom/object.c b/qom/object.c > index 00bb3b0..227d966 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -649,7 +649,12 @@ void object_unref(Object *obj) > > /* parent always holds a reference to its children */ > if (obj->ref == 0) { > - object_finalize(obj); > + /* fixme, maybe introduce obj->finalze to make this more elegant */ > + if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) { hw/qdev.h:#define TYPE_DEVICE "device" This should be object_dynamic_cast(obj, TYPE_DEVICE). Stefan
Il 25/07/2012 09:03, Stefan Hajnoczi ha scritto: > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > > iohandler/bh/timer may use DeviceState when its refcnt=0, It's not clear how to me. The only reference to devices from an iohandler/bh/timer can be in the opaque. Now, if you have a iohandler/bh/timer whose opaque is a DeviceState, you should bump the refcount before setting it up, and unref after tearing it down. See for example how hw/scsi-disk.c bumps the refcount of a request before starting asynchronous I/O and calls unref at the end of the asynchronous I/O callback. Paolo
On Wed, Jul 25, 2012 at 3:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 25/07/2012 09:03, Stefan Hajnoczi ha scritto: >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> >> iohandler/bh/timer may use DeviceState when its refcnt=0, > > It's not clear how to me. The only reference to devices from an > iohandler/bh/timer can be in the opaque. Now, if you have a > iohandler/bh/timer whose opaque is a DeviceState, you should bump the > refcount before setting it up, and unref after tearing it down. > Yes, I admit refcnt is a good solution, but I think it means that we will fix it with each device's bh. And this way seems lazy. Thanx pingfan > See for example how hw/scsi-disk.c bumps the refcount of a request > before starting asynchronous I/O and calls unref at the end of the > asynchronous I/O callback. > > Paolo
On Wed, Jul 25, 2012 at 3:03 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote: >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> >> iohandler/bh/timer may use DeviceState when its refcnt=0, >> postpone the reclaimer till they have done with it. >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> --- >> include/qemu/object.h | 2 +- >> main-loop.c | 4 ++++ >> main-loop.h | 2 ++ >> qemu-tool.c | 4 ++++ >> qom/Makefile.objs | 2 +- >> qom/object.c | 7 ++++++- >> qom/reclaimer.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> 7 files changed, 59 insertions(+), 3 deletions(-) >> create mode 100644 qom/reclaimer.c >> >> diff --git a/include/qemu/object.h b/include/qemu/object.h >> index 8b17776..b233ee4 100644 >> --- a/include/qemu/object.h >> +++ b/include/qemu/object.h >> @@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque), >> */ >> Object *container_get(Object *root, const char *path); >> >> - >> +void qemu_reclaimer_enqueue(Object *obj); >> #endif >> diff --git a/main-loop.c b/main-loop.c >> index eb3b6e6..f9cecc5 100644 >> --- a/main-loop.c >> +++ b/main-loop.c >> @@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking) >> them. */ >> qemu_bh_poll(); >> >> + /* ref to device from iohandler/bh/timer do not obey the rules, so delay >> + * reclaiming until now. >> + */ >> + qemu_device_reclaimer(); >> return ret; >> } >> diff --git a/main-loop.h b/main-loop.h >> index cedddf5..1a59a6d 100644 >> --- a/main-loop.h >> +++ b/main-loop.h >> @@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh); >> int qemu_bh_poll(void); >> void qemu_bh_update_timeout(uint32_t *timeout); >> >> +void qemu_device_reclaimer(void); >> + >> #endif >> diff --git a/qemu-tool.c b/qemu-tool.c >> index 318c5fc..34d959b 100644 >> --- a/qemu-tool.c >> +++ b/qemu-tool.c >> @@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void) >> { >> } >> >> +void qemu_device_reclaimer(void) >> +{ >> +} >> + >> int use_icount; >> >> void qemu_clock_warp(QEMUClock *clock) >> diff --git a/qom/Makefile.objs b/qom/Makefile.objs >> index 5ef060a..a579261 100644 >> --- a/qom/Makefile.objs >> +++ b/qom/Makefile.objs >> @@ -1,4 +1,4 @@ >> -qom-obj-y = object.o container.o qom-qobject.o >> +qom-obj-y = object.o container.o qom-qobject.o reclaimer.o >> qom-obj-twice-y = cpu.o >> common-obj-y = $(qom-obj-twice-y) >> user-obj-y = $(qom-obj-twice-y) >> diff --git a/qom/object.c b/qom/object.c >> index 00bb3b0..227d966 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -649,7 +649,12 @@ void object_unref(Object *obj) >> >> /* parent always holds a reference to its children */ >> if (obj->ref == 0) { >> - object_finalize(obj); >> + /* fixme, maybe introduce obj->finalze to make this more elegant */ >> + if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) { > > hw/qdev.h:#define TYPE_DEVICE "device" > > This should be object_dynamic_cast(obj, TYPE_DEVICE). > Yes, thanks. > Stefan
Il 25/07/2012 10:16, liu ping fan ha scritto: > > It's not clear how to me. The only reference to devices from an > > iohandler/bh/timer can be in the opaque. Now, if you have a > > iohandler/bh/timer whose opaque is a DeviceState, you should bump the > > refcount before setting it up, and unref after tearing it down. > > Yes, I admit refcnt is a good solution, but I think it means that we > will fix it with each device's bh. And this way seems lazy. Most of the time the bh/timer/iohandler is created in the init function, and deleted in the exit function, so in this case the lifecycle is even easier to manage. Looking at your patch again, it seems like you're implementing a poor-man RCU. So that's fine for a proof-of-concept, but let's replace it with real RCU sooner or later. Paolo
diff --git a/include/qemu/object.h b/include/qemu/object.h index 8b17776..b233ee4 100644 --- a/include/qemu/object.h +++ b/include/qemu/object.h @@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque), */ Object *container_get(Object *root, const char *path); - +void qemu_reclaimer_enqueue(Object *obj); #endif diff --git a/main-loop.c b/main-loop.c index eb3b6e6..f9cecc5 100644 --- a/main-loop.c +++ b/main-loop.c @@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking) them. */ qemu_bh_poll(); + /* ref to device from iohandler/bh/timer do not obey the rules, so delay + * reclaiming until now. + */ + qemu_device_reclaimer(); return ret; } diff --git a/main-loop.h b/main-loop.h index cedddf5..1a59a6d 100644 --- a/main-loop.h +++ b/main-loop.h @@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh); int qemu_bh_poll(void); void qemu_bh_update_timeout(uint32_t *timeout); +void qemu_device_reclaimer(void); + #endif diff --git a/qemu-tool.c b/qemu-tool.c index 318c5fc..34d959b 100644 --- a/qemu-tool.c +++ b/qemu-tool.c @@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void) { } +void qemu_device_reclaimer(void) +{ +} + int use_icount; void qemu_clock_warp(QEMUClock *clock) diff --git a/qom/Makefile.objs b/qom/Makefile.objs index 5ef060a..a579261 100644 --- a/qom/Makefile.objs +++ b/qom/Makefile.objs @@ -1,4 +1,4 @@ -qom-obj-y = object.o container.o qom-qobject.o +qom-obj-y = object.o container.o qom-qobject.o reclaimer.o qom-obj-twice-y = cpu.o common-obj-y = $(qom-obj-twice-y) user-obj-y = $(qom-obj-twice-y) diff --git a/qom/object.c b/qom/object.c index 00bb3b0..227d966 100644 --- a/qom/object.c +++ b/qom/object.c @@ -649,7 +649,12 @@ void object_unref(Object *obj) /* parent always holds a reference to its children */ if (obj->ref == 0) { - object_finalize(obj); + /* fixme, maybe introduce obj->finalze to make this more elegant */ + if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) { + qemu_reclaimer_enqueue(obj); + } else { + object_finalize(obj); + } } } diff --git a/qom/reclaimer.c b/qom/reclaimer.c new file mode 100644 index 0000000..2fb3410 --- /dev/null +++ b/qom/reclaimer.c @@ -0,0 +1,41 @@ +/* + * QEMU DeviceState reclaimer + * + * Copyright IBM, Corp. 2012 + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu-common.h" +#include "qemu-thread.h" +#include "main-loop.h" +#include "qemu/object.h" + +typedef struct Chunk { + QLIST_ENTRY(Chunk) list; + Object *obj; +} Chunk; + +static struct QemuMutex reclaimer_lock; +static QLIST_HEAD(rcl, Chunk) reclaimer_list; + +void qemu_reclaimer_enqueue(Object *obj) +{ + Chunk *r = g_malloc0(sizeof(Chunk)); + r->obj = obj; + qemu_mutex_lock(&reclaimer_lock); + QLIST_INSERT_HEAD_RCU(&reclaimer_list, r, list); + qemu_mutex_unlock(&reclaimer_lock); +} + +void qemu_device_reclaimer(void) +{ + Chunk *cur, *next; + + QLIST_FOREACH_SAFE(cur, &reclaimer_list, list, next) { + QLIST_REMOVE(cur, list); + object_finalize(cur->obj); + g_free(cur); + } +}