Re: [Qemu-devel] [PATCH] RAMList: replace QemuMutex with CompatGMutex

[Paolo Bonzini]

On 16/02/2018 10:13, Peter Maydell wrote:
> On 14 February 2018 at 12:11, Paolo Bonzini  wrote:
>> On 14/02/2018 12:43, Zihan Yang wrote:
>>>
 I'm not sure what is the advantage of this change.  QEMU is more or less
 uniformly using QemuMutex and QemuCond.
>>>
>>> I see. QEMU uses some glib functions, like g_new, g_free, g_hash_table ,
>>> g_assert and g_poll, so I thought there was a trend for making more use
>>> of glib.
>>> I get the point now.
>>
>> There are some plans about using priority inheritance mutexes for
>> QemuMutex.  GLib does not support anymore pluggable thread functions, so
>> we are stuck with our own implementation.
> 
> Should we remove the entry
> "Change QemuMutex and QemuCond to CompatGMutex and CompatGCond (these are
>  the same as GMutex and GCond, just with a different type). With this change,
>  qemu_mutex_init/qemu_cond_init becomes optional for global variables."
> from https://wiki.qemu.org/Contribute/BiteSizedTasks then?

Yes, good idea.

Paolo

Re: [Qemu-devel] [PATCH] RAMList: replace QemuMutex with CompatGMutex

[Peter Maydell]

On 14 February 2018 at 12:11, Paolo Bonzini  wrote:
> On 14/02/2018 12:43, Zihan Yang wrote:
>>
>>> I'm not sure what is the advantage of this change.  QEMU is more or less
>>> uniformly using QemuMutex and QemuCond.
>>
>> I see. QEMU uses some glib functions, like g_new, g_free, g_hash_table ,
>> g_assert and g_poll, so I thought there was a trend for making more use
>> of glib.
>> I get the point now.
>
> There are some plans about using priority inheritance mutexes for
> QemuMutex.  GLib does not support anymore pluggable thread functions, so
> we are stuck with our own implementation.

Should we remove the entry
"Change QemuMutex and QemuCond to CompatGMutex and CompatGCond (these are
 the same as GMutex and GCond, just with a different type). With this change,
 qemu_mutex_init/qemu_cond_init becomes optional for global variables."
from https://wiki.qemu.org/Contribute/BiteSizedTasks then?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] RAMList: replace QemuMutex with CompatGMutex

[Paolo Bonzini]

On 14/02/2018 12:43, Zihan Yang wrote:
> 
>> I'm not sure what is the advantage of this change.  QEMU is more or less
>> uniformly using QemuMutex and QemuCond.
> 
> I see. QEMU uses some glib functions, like g_new, g_free, g_hash_table ,
> g_assert and g_poll, so I thought there was a trend for making more use
> of glib.
> I get the point now.

There are some plans about using priority inheritance mutexes for
QemuMutex.  GLib does not support anymore pluggable thread functions, so
we are stuck with our own implementation.

Paolo

> 2018-02-14 18:13 GMT+08:00 Paolo Bonzini  >:
> 
> On 14/02/2018 09:49, Zihan Yang wrote:
> > change the mutex member in RAMList structure from QemuMutex to 
> CompatGMutex.
> > qemu_mutex_init() is just deleted instead of being replaced with 
> g_mutex_init()
> > because there is no need to do initialize a mutex that is statically 
> allocated.
> >
> > Signed-off-by: Zihan Yang  >
> 
> I'm not sure what is the advantage of this change.  QEMU is more or less
> uniformly using QemuMutex and QemuCond.
> 
> Paolo
> 
> > ---
> >  exec.c                 | 5 ++---
> >  include/exec/ramlist.h | 2 +-
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index e8d7b33..90cdd05 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1423,12 +1423,12 @@ void qemu_flush_coalesced_mmio_buffer(void)
> >
> >  void qemu_mutex_lock_ramlist(void)
> >  {
> > -    qemu_mutex_lock(_list.mutex);
> > +    g_mutex_lock(_list.mutex);
> >  }
> >
> >  void qemu_mutex_unlock_ramlist(void)
> >  {
> > -    qemu_mutex_unlock(_list.mutex);
> > +    g_mutex_unlock(_list.mutex);
> >  }
> >
> >  void ram_block_dump(Monitor *mon)
> > @@ -3331,7 +3331,6 @@ void cpu_register_map_client(QEMUBH *bh)
> >
> >  void cpu_exec_init_all(void)
> >  {
> > -    qemu_mutex_init(_list.mutex);
> >      /* The data structures we set up here depend on knowing the
> page size,
> >       * so no more changes can be made after this point.
> >       * In an ideal world, nothing we did before we had finished the
> > diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> > index 2e2ac6c..24be5e4 100644
> > --- a/include/exec/ramlist.h
> > +++ b/include/exec/ramlist.h
> > @@ -45,7 +45,7 @@ typedef struct {
> >  } DirtyMemoryBlocks;
> >
> >  typedef struct RAMList {
> > -    QemuMutex mutex;
> > +    CompatGMutex mutex;
> >      RAMBlock *mru_block;
> >      /* RCU-enabled, writes protected by the ramlist lock. */
> >      QLIST_HEAD(, RAMBlock) blocks;
> >
> 
> 

Re: [Qemu-devel] [PATCH] RAMList: replace QemuMutex with CompatGMutex

[Zihan Yang]

> I'm not sure what is the advantage of this change.  QEMU is more or less
> uniformly using QemuMutex and QemuCond.

I see. QEMU uses some glib functions, like g_new, g_free, g_hash_table ,
g_assert and g_poll, so I thought there was a trend for making more use of
glib.
I get the point now.


2018-02-14 18:13 GMT+08:00 Paolo Bonzini :

> On 14/02/2018 09:49, Zihan Yang wrote:
> > change the mutex member in RAMList structure from QemuMutex to
> CompatGMutex.
> > qemu_mutex_init() is just deleted instead of being replaced with
> g_mutex_init()
> > because there is no need to do initialize a mutex that is statically
> allocated.
> >
> > Signed-off-by: Zihan Yang 
>
> I'm not sure what is the advantage of this change.  QEMU is more or less
> uniformly using QemuMutex and QemuCond.
>
> Paolo
>
> > ---
> >  exec.c | 5 ++---
> >  include/exec/ramlist.h | 2 +-
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index e8d7b33..90cdd05 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1423,12 +1423,12 @@ void qemu_flush_coalesced_mmio_buffer(void)
> >
> >  void qemu_mutex_lock_ramlist(void)
> >  {
> > -qemu_mutex_lock(_list.mutex);
> > +g_mutex_lock(_list.mutex);
> >  }
> >
> >  void qemu_mutex_unlock_ramlist(void)
> >  {
> > -qemu_mutex_unlock(_list.mutex);
> > +g_mutex_unlock(_list.mutex);
> >  }
> >
> >  void ram_block_dump(Monitor *mon)
> > @@ -3331,7 +3331,6 @@ void cpu_register_map_client(QEMUBH *bh)
> >
> >  void cpu_exec_init_all(void)
> >  {
> > -qemu_mutex_init(_list.mutex);
> >  /* The data structures we set up here depend on knowing the page
> size,
> >   * so no more changes can be made after this point.
> >   * In an ideal world, nothing we did before we had finished the
> > diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> > index 2e2ac6c..24be5e4 100644
> > --- a/include/exec/ramlist.h
> > +++ b/include/exec/ramlist.h
> > @@ -45,7 +45,7 @@ typedef struct {
> >  } DirtyMemoryBlocks;
> >
> >  typedef struct RAMList {
> > -QemuMutex mutex;
> > +CompatGMutex mutex;
> >  RAMBlock *mru_block;
> >  /* RCU-enabled, writes protected by the ramlist lock. */
> >  QLIST_HEAD(, RAMBlock) blocks;
> >
>
>

Re: [Qemu-devel] [PATCH] RAMList: replace QemuMutex with CompatGMutex

[Paolo Bonzini]

On 14/02/2018 09:49, Zihan Yang wrote:
> change the mutex member in RAMList structure from QemuMutex to CompatGMutex.
> qemu_mutex_init() is just deleted instead of being replaced with 
> g_mutex_init()
> because there is no need to do initialize a mutex that is statically 
> allocated.
> 
> Signed-off-by: Zihan Yang 

I'm not sure what is the advantage of this change.  QEMU is more or less
uniformly using QemuMutex and QemuCond.

Paolo

> ---
>  exec.c | 5 ++---
>  include/exec/ramlist.h | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index e8d7b33..90cdd05 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1423,12 +1423,12 @@ void qemu_flush_coalesced_mmio_buffer(void)
>  
>  void qemu_mutex_lock_ramlist(void)
>  {
> -qemu_mutex_lock(_list.mutex);
> +g_mutex_lock(_list.mutex);
>  }
>  
>  void qemu_mutex_unlock_ramlist(void)
>  {
> -qemu_mutex_unlock(_list.mutex);
> +g_mutex_unlock(_list.mutex);
>  }
>  
>  void ram_block_dump(Monitor *mon)
> @@ -3331,7 +3331,6 @@ void cpu_register_map_client(QEMUBH *bh)
>  
>  void cpu_exec_init_all(void)
>  {
> -qemu_mutex_init(_list.mutex);
>  /* The data structures we set up here depend on knowing the page size,
>   * so no more changes can be made after this point.
>   * In an ideal world, nothing we did before we had finished the
> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> index 2e2ac6c..24be5e4 100644
> --- a/include/exec/ramlist.h
> +++ b/include/exec/ramlist.h
> @@ -45,7 +45,7 @@ typedef struct {
>  } DirtyMemoryBlocks;
>  
>  typedef struct RAMList {
> -QemuMutex mutex;
> +CompatGMutex mutex;
>  RAMBlock *mru_block;
>  /* RCU-enabled, writes protected by the ramlist lock. */
>  QLIST_HEAD(, RAMBlock) blocks;
>