| Message ID | 1344407156-25562-4-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 08/08/2012 09:25 AM, Liu Ping Fan wrote: > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > > Collect unused object and release them at caller demand. > Please explain the motivation for this patch.
Il 08/08/2012 11:05, Avi Kivity ha scritto: >> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> > >> > Collect unused object and release them at caller demand. >> > > Please explain the motivation for this patch. It's poor man RCU, I think? Paolo
On 08/08/2012 12:07 PM, Paolo Bonzini wrote: > Il 08/08/2012 11:05, Avi Kivity ha scritto: >>> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >>> > >>> > Collect unused object and release them at caller demand. >>> > >> Please explain the motivation for this patch. > > It's poor man RCU, I think? I thought that it was to defer destructors (finalizers) to a more suitable context. But why is the unref context unsuitable? I don't see how it relates to RCU, where is the C and the U? Anyway the list eagerly awaits the explanation.
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? > +#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? > +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. > +} >
On Wed, Aug 8, 2012 at 5:15 PM, Avi Kivity <avi@redhat.com> wrote: > On 08/08/2012 12:07 PM, Paolo Bonzini wrote: >> Il 08/08/2012 11:05, Avi Kivity ha scritto: >>>> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >>>> > >>>> > Collect unused object and release them at caller demand. >>>> > >>> Please explain the motivation for this patch. >> >> It's poor man RCU, I think? > > I thought that it was to defer destructors (finalizers) to a more > suitable context. But why is the unref context unsuitable? > Yes, it is to defer destructors. See 0009-memory-prepare-flatview-and-radix-tree-for-rcu-style.patch When MemoryRegion is _del_subregion from mem in updater, it may be still in use by reader -- radix or flatview, so defer its destructors to the reclaimer --phys_map_release(PhysMap *map) If we have rcu, it could be elegant to do this. I think, I should write the commit comment here too, not until the followed patch. Regards, pingfan > I don't see how it relates to RCU, where is the C and the U? > > Anyway the list eagerly awaits the explanation. > > -- > error compiling committee.c: too many arguments to function
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. > >> +} >> > >
Il 09/08/2012 09:33, liu ping fan ha scritto: > Yes, it is to defer destructors. > See 0009-memory-prepare-flatview-and-radix-tree-for-rcu-style.patch > When MemoryRegion is _del_subregion from mem in updater, it may be > still in use by reader -- radix or flatview, so defer its destructors > to the reclaimer --phys_map_release(PhysMap *map) How are you sure that the reader is already out of its critical section by the time the reclaimer runs? > If we have rcu, it could be elegant to do this. Yeah, I think inventing primitives is dangerous and difficult to review; and it may be difficult to replace it with proper call_rcu. You should probably make a proof-of-concept using liburcu. Then we can decide how to implement RCU in a way that is portable enough for QEMU's needs. Paolo
On 08/09/2012 10:49 AM, Paolo Bonzini wrote: > Il 09/08/2012 09:33, liu ping fan ha scritto: >> Yes, it is to defer destructors. >> See 0009-memory-prepare-flatview-and-radix-tree-for-rcu-style.patch >> When MemoryRegion is _del_subregion from mem in updater, it may be >> still in use by reader -- radix or flatview, so defer its destructors >> to the reclaimer --phys_map_release(PhysMap *map) > > How are you sure that the reader is already out of its critical section > by the time the reclaimer runs? > >> If we have rcu, it could be elegant to do this. > > Yeah, I think inventing primitives is dangerous and difficult to review; > and it may be difficult to replace it with proper call_rcu. > > You should probably make a proof-of-concept using liburcu. Then we can > decide how to implement RCU in a way that is portable enough for QEMU's > needs. IMO we should start with a simple mutex (which will cover only the lookup and map rebuild). This should reduce the contention to basically nothing (still leaving a cache line bounce). If a profile shows the cache line bounce hurting us, or perhaps contention in ultralarge guests, then we should switch to rcu.
On Thu, Aug 9, 2012 at 4:18 PM, Avi Kivity <avi@redhat.com> wrote: > On 08/09/2012 10:49 AM, Paolo Bonzini wrote: >> Il 09/08/2012 09:33, liu ping fan ha scritto: >>> Yes, it is to defer destructors. >>> See 0009-memory-prepare-flatview-and-radix-tree-for-rcu-style.patch >>> When MemoryRegion is _del_subregion from mem in updater, it may be >>> still in use by reader -- radix or flatview, so defer its destructors >>> to the reclaimer --phys_map_release(PhysMap *map) >> >> How are you sure that the reader is already out of its critical section >> by the time the reclaimer runs? >> >>> If we have rcu, it could be elegant to do this. >> >> Yeah, I think inventing primitives is dangerous and difficult to review; >> and it may be difficult to replace it with proper call_rcu. >> >> You should probably make a proof-of-concept using liburcu. Then we can >> decide how to implement RCU in a way that is portable enough for QEMU's >> needs. > > IMO we should start with a simple mutex (which will cover only the > lookup and map rebuild). This should reduce the contention to basically > nothing (still leaving a cache line bounce). If a profile shows the > cache line bounce hurting us, or perhaps contention in ultralarge > guests, then we should switch to rcu. > Agree, I think this will pin us on the major issue -- mmio perfermance Regards, pingfan > > -- > error compiling committee.c: too many arguments to function
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); +#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); +} + +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); + } +} + +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); + } +}