Re: [Qemu-devel] [PATCH v1 1/2] reduce qemu's heap Rss size from 12252kB to 2752KB

[Xu, Anthony]

> -----Original Message-----
> From: Paolo Bonzini [mailto:address@hidden
> Sent: Friday, March 10, 2017 12:42 AM
> To: Zhong, Yang <address@hidden>; address@hidden
> Cc: Xu, Anthony <address@hidden>; Peng, Chao P
> <address@hidden>
> Subject: Re: [PATCH v1 1/2] reduce qemu's heap Rss size from 12252kB to
> 2752KB
> 
> 
> 
> On 10/03/2017 16:14, Yang Zhong wrote:
> > There are a lot of memory allocation during the qemu bootup, which are
> > freed later by RCU thread,which means the heap size becomes biger and
> > biger when allocation happens, but the heap may not be shrinked even
> > after release happens,because some memory blocks in top of heap are
> > still being used.Decreasing the sleep and removing
> > qemu_mutex_unlock_iothread() lock, which make call_rcu_thread()thread
> response the free memory in time.
> > This patch will reduce heap Rss around 10M.
> >
> > This patch is from Anthony xu <address@hidden>.
> >
> > Signed-off-by: Yang Zhong <address@hidden>
> > ---
> >  util/rcu.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/util/rcu.c b/util/rcu.c
> > index 9adc5e4..c5c373c 100644
> > --- a/util/rcu.c
> > +++ b/util/rcu.c
> > @@ -167,7 +167,7 @@ void synchronize_rcu(void)  }
> >
> >
> > -#define RCU_CALL_MIN_SIZE        30
> > +#define RCU_CALL_MIN_SIZE        5
> >
> >  /* Multi-producer, single-consumer queue based on
> urcu/static/wfqueue.h
> >   * from liburcu.  Note that head is only used by the consumer.
> > @@ -241,7 +241,7 @@ static void *call_rcu_thread(void *opaque)
> >           * added before synchronize_rcu() starts.
> >           */
> >          while (n == 0 || (n < RCU_CALL_MIN_SIZE && ++tries <= 5)) {
> > -            g_usleep(10000);
> > +            g_usleep(100);
> >              if (n == 0) {
> >                  qemu_event_reset(&rcu_call_ready_event);
> >                  n = atomic_read(&rcu_call_count); @@ -254,24 +254,20
> > @@ static void *call_rcu_thread(void *opaque)
> >
> >          atomic_sub(&rcu_call_count, n);
> >          synchronize_rcu();
> > -        qemu_mutex_lock_iothread();
> >          while (n > 0) {
> >              node = try_dequeue();
> >              while (!node) {
> > -                qemu_mutex_unlock_iothread();
> >                  qemu_event_reset(&rcu_call_ready_event);
> >                  node = try_dequeue();
> >                  if (!node) {
> >                      qemu_event_wait(&rcu_call_ready_event);
> >                      node = try_dequeue();
> >                  }
> > -                qemu_mutex_lock_iothread();
> >              }
> >
> >              n--;
> >              node->func(node);
> >          }
> > -        qemu_mutex_unlock_iothread();
> 
> This is wrong.  RCU callbacks currently need the "big QEMU lock", because
> they can free arbitrary QOM objects (including MemoryRegions).

Using "big QEMU lock" in RCU callbacks is a little bit heavy, we'd like to 
remove it.

We noticed an issue related to MemoryRegion popping up after we removed the 
global lock.
The root cause is in MemoryRegion destruction memory_region_transaction_commit 
is called, which may cause issue.
Freeing MemoryRegion seems not cause any issue.
Patch2 in this series fixes this issue,
We found out only subpage MemoryRegion is freed in RCU callbacks, and 
memory_region_transaction_commit
is not needed for subpage MemoryRegion because it doesn't have sub regions.

Ideally, freeing QOM object should not require a global lock. 
If you see any other QOM requiring a global lock, please let us know, we are 
willing to fix it.


Thanks,
Anthony


> 
> Paolo
> 
> >      }
> >      abort();
> >  }
> >