Re: [PATCH 03/15] qom: introduce reclaimer to release obj

[liu ping fan <qemulist@gmail.com>]

On Wed, Aug 8, 2012 at 5:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Collect unused object and release them at caller demand.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  include/qemu/reclaimer.h |   28 ++++++++++++++++++++++
>>  main-loop.c              |    5 ++++
>>  qemu-tool.c              |    5 ++++
>>  qom/Makefile.objs        |    2 +-
>>  qom/reclaimer.c          |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 97 insertions(+), 1 deletions(-)
>>  create mode 100644 include/qemu/reclaimer.h
>>  create mode 100644 qom/reclaimer.c
>>
>> diff --git a/include/qemu/reclaimer.h b/include/qemu/reclaimer.h
>> new file mode 100644
>> index 0000000..9307e93
>> --- /dev/null
>> +++ b/include/qemu/reclaimer.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * QEMU 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.
>> + */
>> +
>> +#ifndef QEMU_RECLAIMER
>> +#define QEMU_RECLAIMER
>> +
>> +typedef void ReleaseHandler(void *opaque);
>> +typedef struct Chunk {
>> +    QLIST_ENTRY(Chunk) list;
>> +    void *opaque;
>> +    ReleaseHandler *release;
>> +} Chunk;
>> +
>> +typedef struct ChunkHead {
>> +        struct Chunk *lh_first;
>> +} ChunkHead;
>> +
>> +void reclaimer_enqueue(ChunkHead *head, void *opaque, ReleaseHandler *release);
>> +void reclaimer_worker(ChunkHead *head);
>> +void qemu_reclaimer_enqueue(void *opaque, ReleaseHandler *release);
>> +void qemu_reclaimer(void);
>
> So "enqueue" is call_rcu and qemu_reclaimer marks a quiescent state +
> empties the pending call_rcu.
>
> But what's the difference between the two pairs of APIs?
>
I add the new one for V2 to reclaim the resource for mem view. And
yes, as you point out, I will delete the 2nd API, for it can be
substituted by the 1st one easily.

>> +#endif
>> diff --git a/main-loop.c b/main-loop.c
>> index eb3b6e6..be9d095 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -26,6 +26,7 @@
>>  #include "qemu-timer.h"
>>  #include "slirp/slirp.h"
>>  #include "main-loop.h"
>> +#include "qemu/reclaimer.h"
>>
>>  #ifndef _WIN32
>>
>> @@ -505,5 +506,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_reclaimer();
>>      return ret;
>>  }
>> diff --git a/qemu-tool.c b/qemu-tool.c
>> index 318c5fc..f5fe319 100644
>> --- a/qemu-tool.c
>> +++ b/qemu-tool.c
>> @@ -21,6 +21,7 @@
>>  #include "main-loop.h"
>>  #include "qemu_socket.h"
>>  #include "slirp/libslirp.h"
>> +#include "qemu/reclaimer.h"
>>
>>  #include <sys/time.h>
>>
>> @@ -75,6 +76,10 @@ void qemu_mutex_unlock_iothread(void)
>>  {
>>  }
>>
>> +void qemu_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/reclaimer.c b/qom/reclaimer.c
>> new file mode 100644
>> index 0000000..6cb53e3
>> --- /dev/null
>> +++ b/qom/reclaimer.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * QEMU 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-queue.h"
>> +#include "qemu/reclaimer.h"
>> +
>> +static struct QemuMutex reclaimer_lock;
>> +static QLIST_HEAD(rcl, Chunk) reclaimer_list;
>> +
>> +void reclaimer_enqueue(ChunkHead *head, void *opaque, ReleaseHandler *release)
>> +{
>> +    Chunk *r = g_malloc0(sizeof(Chunk));
>> +    r->opaque = opaque;
>> +    r->release = release;
>> +    QLIST_INSERT_HEAD_RCU(head, r, list);
>> +}
>
> No lock?
Yes, need!  I will think it more closely.

Thanks and regards,
pingfan
>
>> +void reclaimer_worker(ChunkHead *head)
>> +{
>> +    Chunk *cur, *next;
>> +
>> +    QLIST_FOREACH_SAFE(cur, head, list, next) {
>> +        QLIST_REMOVE(cur, list);
>> +        cur->release(cur->opaque);
>> +        g_free(cur);
>> +    }
>
> QLIST_REMOVE needs a lock too, so using the lockless
> QLIST_INSERT_HEAD_RCU is not necessary.
>
>> +}
>> +
>> +void qemu_reclaimer_enqueue(void *opaque, ReleaseHandler *release)
>> +{
>> +    Chunk *r = g_malloc0(sizeof(Chunk));
>> +    r->opaque = opaque;
>> +    r->release = release;
>> +    qemu_mutex_lock(&reclaimer_lock);
>> +    QLIST_INSERT_HEAD_RCU(&reclaimer_list, r, list);
>> +    qemu_mutex_unlock(&reclaimer_lock);
>> +}
>> +
>> +
>> +void qemu_reclaimer(void)
>> +{
>> +    Chunk *cur, *next;
>> +
>> +    QLIST_FOREACH_SAFE(cur, &reclaimer_list, list, next) {
>> +        QLIST_REMOVE(cur, list);
>> +        cur->release(cur->opaque);
>> +        g_free(cur);
>> +    }
>
> Same here.
>
>> +}
>>
>
>