Patchwork [4/5] qom: delay DeviceState's reclaim to main-loop

login
register
mail settings
Submitter pingfan liu
Date July 25, 2012, 3:31 a.m.
Message ID <1343187070-27371-5-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/173101/
State New
Headers show

Comments

pingfan liu - July 25, 2012, 3:31 a.m.
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
Stefan Hajnoczi - July 25, 2012, 7:03 a.m.
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
Paolo Bonzini - July 25, 2012, 7:37 a.m.
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
pingfan liu - July 25, 2012, 8:16 a.m.
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
pingfan liu - July 25, 2012, 8:17 a.m.
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
Paolo Bonzini - July 25, 2012, 8:22 a.m.
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

Patch

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);
+    }
+}