[openib-general] RE: [PATCH] opensm: make cl_atomic functions atomic

[Eitan Zahavi]

Nice catch !
> 
> complib's cl_atomic_() functions are not atomic in current
implementation,
> they use static per function locks so race is possible between
different
> functions (for example between _inc() and _dec()). Such races are
pretty
> reproducible in practice - for example it is used to inc/decrement mad
> counters in opensm.
> 
> This patch is fast workaround. I think we will need to review usage
and
> implementation of atomic stuff in opensm for better solution.
> 
> Sasha.
> 
> 
> This is workaround for broken cl_atomic_() stuff used in
complib/OpenSM:
> it replaces static per function locks by one globally shared lock.
> In long term we will need better solution.
> 
> Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
> ---
> 
>  osm/complib/cl_complib.c            |   44 ++++++++--------------
>  osm/complib/libosmcomp.map          |    1 +
>  osm/include/complib/cl_atomic_osd.h |   69
++++++++++-------------------------
>  3 files changed, 36 insertions(+), 78 deletions(-)
> 
> diff --git a/osm/complib/cl_complib.c b/osm/complib/cl_complib.c
> index 9eb475b..e33dfef 100644
> --- a/osm/complib/cl_complib.c
> +++ b/osm/complib/cl_complib.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2004, 2005 Voltaire, Inc. All rights reserved.
> + * Copyright (c) 2004-2006 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights
reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   *
> @@ -40,6 +40,7 @@
>  #endif /* HAVE_CONFIG_H */
> 
>  #include <complib/cl_types.h>
> +#include <complib/cl_spinlock.h>
>  #include <complib/cl_device.h>
>  #include <complib/cl_debug.h>
>  #include <complib/cl_syshelper.h>
> @@ -61,14 +62,7 @@ extern
>  void
>  __cl_timer_prov_destroy( void );
> 
> -void
> -complib_exit(void);
> -
> -void
> -complib_fini(void);
> -
> -void
> -complib_init(void);
> +cl_spinlock_t cl_atomic_spinlock;
> 
>  void
>  __attribute (( constructor ))
> @@ -76,28 +70,23 @@ complib_init(void)
>  {
>  	cl_status_t	status = CL_SUCCESS;
> 
> -	/*
> -	 *  System Helper Init.
> -	 */
> +  	status = cl_spinlock_init(&cl_atomic_spinlock);
> +	if (status != CL_SUCCESS)
> +		goto _error;
> +
>  	status = __cl_user_syshelper_init();
>  	if( status != CL_SUCCESS )
> -	{
> -		cl_msg_out( "__init: failed to init syshelper (%s)\n",
> -
CL_STATUS_MSG(
> status ) );
> -		exit(1);
> -	}
> -
> -	/*
> -	 *  Timer Init
> -	 */
> +		goto _error;
> 
>  	status = __cl_timer_prov_create();
>  	if( status != CL_SUCCESS)
> -	{
> -		cl_msg_out( "__init: failed to create timer provider
status (%s)\n",
> -
CL_STATUS_MSG(
> status ) );
> -		exit(1);
> -	}
> +		goto _error;
> +	return;
> +
> + _error:
> +	cl_msg_out( "__init: failed to create complib (%s)\n",
> +					CL_STATUS_MSG( status ) );
> +	exit(1);
>  }
> 
>  void
> @@ -105,7 +94,6 @@ __attribute (( destructor ))
>  complib_fini(void)
>  {
>    __cl_timer_prov_destroy();
> -
>    __cl_user_syshelper_exit();
>  }
> 
> @@ -113,8 +101,8 @@ void
>  complib_exit(void)
>  {
>    __cl_timer_prov_destroy();
> -
>    __cl_user_syshelper_exit();
> +  cl_spinlock_destroy(&cl_atomic_spinlock);
>  }
> 
>  boolean_t
> diff --git a/osm/complib/libosmcomp.map b/osm/complib/libosmcomp.map
> index 8d5f38c..e61ee57 100644
> --- a/osm/complib/libosmcomp.map
> +++ b/osm/complib/libosmcomp.map
> @@ -200,6 +200,7 @@ OSMCOMP_1.0 {
>  		cl_signal_wait_object;
>  		cl_destroy_wait_object;
>  		cl_clear_wait_object;
> +		cl_atomic_spinlock;
>  		cl_atomic_dec;
>  		cl_free;
>  		cl_malloc;
> diff --git a/osm/include/complib/cl_atomic_osd.h
> b/osm/include/complib/cl_atomic_osd.h
> index a592a8e..ad7ce11 100644
> --- a/osm/include/complib/cl_atomic_osd.h
> +++ b/osm/include/complib/cl_atomic_osd.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2004, 2005 Voltaire, Inc. All rights reserved.
> + * Copyright (c) 2004-2006 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights
reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   *
> @@ -64,23 +64,17 @@
> 
>  BEGIN_C_DECLS
> 
> +extern cl_spinlock_t cl_atomic_spinlock;
> +
>  static inline int32_t
>  cl_atomic_inc(
>  	IN	atomic32_t* const p_value )
>  {
>  	int32_t		new_val;
> -   static      cl_spinlock_t spinlock;
> -
> -   if (spinlock.state != CL_INITIALIZED)
> -   {
> -     cl_spinlock_construct(&spinlock);
> -     cl_spinlock_init(&spinlock);
> -   }
> -
> -   cl_spinlock_acquire(&spinlock);
> -   new_val = *p_value + 1;
> -   *p_value = new_val;
> -   cl_spinlock_release(&spinlock);
> +	cl_spinlock_acquire(&cl_atomic_spinlock);
> +	new_val = *p_value + 1;
> +	*p_value = new_val;
> +	cl_spinlock_release(&cl_atomic_spinlock);
>  	return( new_val );
>  }
> 
> @@ -89,18 +83,10 @@ cl_atomic_dec(
>  	IN	atomic32_t* const	p_value )
>  {
>  	int32_t		new_val;
> -   static      cl_spinlock_t spinlock;
> -
> -   if (spinlock.state != CL_INITIALIZED)
> -   {
> -     cl_spinlock_construct(&spinlock);
> -     cl_spinlock_init(&spinlock);
> -   }
> -
> -   cl_spinlock_acquire(&spinlock);
> -   new_val = *p_value - 1;
> -   *p_value = new_val;
> -   cl_spinlock_release(&spinlock);
> +	cl_spinlock_acquire(&cl_atomic_spinlock);
> +	new_val = *p_value - 1;
> +	*p_value = new_val;
> +	cl_spinlock_release(&cl_atomic_spinlock);
>  	return( new_val );
>  }
> 
> @@ -110,19 +96,10 @@ cl_atomic_add(
>  	IN	const int32_t		increment )
>  {
>  	int32_t		new_val;
> -
> -   static      cl_spinlock_t spinlock;
> -
> -   if (spinlock.state != CL_INITIALIZED)
> -   {
> -     cl_spinlock_construct(&spinlock);
> -     cl_spinlock_init(&spinlock);
> -   }
> -
> -   cl_spinlock_acquire(&spinlock);
> -   new_val = *p_value + increment;
> -   *p_value = new_val;
> -   cl_spinlock_release(&spinlock);
> +	cl_spinlock_acquire(&cl_atomic_spinlock);
> +	new_val = *p_value + increment;
> +	*p_value = new_val;
> +	cl_spinlock_release(&cl_atomic_spinlock);
>  	return( new_val );
>  }
> 
> @@ -132,18 +109,10 @@ cl_atomic_sub(
>  	IN	const int32_t		decrement )
>  {
>  	int32_t		new_val;
> -   static      cl_spinlock_t spinlock;
> -
> -   if (spinlock.state != CL_INITIALIZED)
> -   {
> -     cl_spinlock_construct(&spinlock);
> -     cl_spinlock_init(&spinlock);
> -   }
> -
> -   cl_spinlock_acquire(&spinlock);
> -   new_val = *p_value + decrement;
> -   *p_value = new_val;
> -   cl_spinlock_release(&spinlock);
> +	cl_spinlock_acquire(&cl_atomic_spinlock);
> +	new_val = *p_value + decrement;
> +	*p_value = new_val;
> +	cl_spinlock_release(&cl_atomic_spinlock);
>  	return( new_val );
>  }
>