Re: [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances

[Jan Kiszka]

On 2013-04-17 10:39, Liu Ping Fan wrote:
> From: Liu Ping Fan <address@hidden>
> 
> slirps will run on dedicated thread, so need to protect the global
> list.
> 
> Signed-off-by: Liu Ping Fan <address@hidden>
> ---
>  include/qemu/module.h |    2 ++
>  slirp/slirp.c         |   20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index c4ccd57..2720943 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -22,6 +22,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
> function(void) {  \
>  
>  typedef enum {
>      MODULE_INIT_BLOCK,
> +    MODULE_INIT_SLIRP,
>      MODULE_INIT_MACHINE,
>      MODULE_INIT_QAPI,
>      MODULE_INIT_QOM,
> @@ -29,6 +30,7 @@ typedef enum {
>  } module_init_type;
>  
>  #define block_init(function) module_init(function, MODULE_INIT_BLOCK)
> +#define slirplayer_init(function) module_init(function, MODULE_INIT_SLIRP)
>  #define machine_init(function) module_init(function, MODULE_INIT_MACHINE)
>  #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
>  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 6bfcc67..4cbf04d 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -42,6 +42,7 @@ static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 
> 0, 0 };
>  
>  u_int curtime;
>  
> +static QemuMutex slirp_instances_lock;
>  static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
>      QTAILQ_HEAD_INITIALIZER(slirp_instances);
>  
> @@ -236,14 +237,18 @@ Slirp *slirp_init(int restricted, struct in_addr 
> vnetwork,
>      register_savevm(NULL, "slirp", 0, 3,
>                      slirp_state_save, slirp_state_load, slirp);
>  
> +    qemu_mutex_lock(&slirp_instances_lock);
>      QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
> +    qemu_mutex_unlock(&slirp_instances_lock);
>  
>      return slirp;
>  }
>  
>  void slirp_cleanup(Slirp *slirp)
>  {
> +    qemu_mutex_lock(&slirp_instances_lock);
>      QTAILQ_REMOVE(&slirp_instances, slirp, entry);
> +    qemu_mutex_unlock(&slirp_instances_lock);
>  
>      unregister_savevm(NULL, "slirp", slirp);
>  
> @@ -262,9 +267,12 @@ void slirp_cleanup(Slirp *slirp)
>  
>  void slirp_update_timeout(uint32_t *timeout)
>  {
> +    qemu_mutex_lock(&slirp_instances_lock);
>      if (!QTAILQ_EMPTY(&slirp_instances)) {
>          *timeout = MIN(1000, *timeout);
>      }
> +    qemu_mutex_unlock(&slirp_instances_lock);
> +
>      curtime = qemu_get_clock_ms(rt_clock);
>  }
>  
> @@ -1140,3 +1148,15 @@ static int slirp_state_load(QEMUFile *f, void *opaque, 
> int version_id)
>  
>      return 0;
>  }
> +
> +static void slirplayer_cleanup(void)
> +{
> +    qemu_mutex_destroy(&slirp_instances_lock);
> +}
> +
> +static void slirplayer_bootup(void)
> +{
> +    qemu_mutex_init(&slirp_instances_lock);
> +    atexit(&slirplayer_cleanup);
> +}
> +slirplayer_init(slirplayer_bootup)
> 

grep'ing for slirp_instances points to more spots that work with that
list (QTAILQ_FOREACH, QTAILQ_EMPTY, ...). So the same question here:
What are the usage rules? When do I _not_ need it when touching the list
of instances, and why?

Well, I started reading at the top, but there are more lock-adding
patches in this series. And the more locks we have, the higher the
probability of ABBA gets. Therefore, please document from the beginning
the lock order rules that shall prevent it (which may also be "never
take other locks while holding this one" or "never hold other locks when
taking this one").

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux