Re: [PATCH v18 8/8] eal: implement functions for mutex management

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> Add functions for mutex init, destroy, lock, unlock, trylock.
> 
> Windows does not have a static initializer. Initialization
> is only done through InitializeCriticalSection(). To overcome this,
> RTE_INIT_MUTEX macro is added to replace static initialization
> of mutexes. The macro calls rte_thread_mutex_init().
> 
> Add unit tests to verify that the mutex correctly locks/unlocks
> and protects the data. Check both static and dynamic mutexes.
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>

Few comments from me below.
I am not sure was such approach already discussed,
if so - apologies for repetition. 

> ---
>  app/test/test_threads.c      | 106 +++++++++++++++++++++++++++++++++++
>  lib/eal/common/rte_thread.c  |  69 +++++++++++++++++++++++
>  lib/eal/include/rte_thread.h |  85 ++++++++++++++++++++++++++++
>  lib/eal/version.map          |   5 ++
>  lib/eal/windows/rte_thread.c |  64 +++++++++++++++++++++
>  5 files changed, 329 insertions(+)
> 
>  };
> diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> index d30a8a7ca3..4a9a1b6e07 100644
> --- a/lib/eal/common/rte_thread.c
> +++ b/lib/eal/common/rte_thread.c
> @@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id)
>  	return pthread_detach((pthread_t)thread_id.opaque_id);
>  }
> 
> +int
> +rte_thread_mutex_init(rte_thread_mutex *mutex)

Don't we need some sort of mutex_attr here too?
To be able to create PROCESS_SHARED mutexes?

> +{
> +	int ret = 0;
> +	pthread_mutex_t *m = NULL;
> +
> +	RTE_VERIFY(mutex != NULL);
> +
> +	m = calloc(1, sizeof(*m));

But is that what we really want for the mutexes?
It means actual mutex will always be allocated on process heap,
away from the data it is supposed to guard.
Even if we'll put performance considerations away,
that wouldn't work for MP case.
Is that considered as ok?

> +	if (m == NULL) {
> +		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
> +		ret = ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	ret = pthread_mutex_init(m, NULL);
> +	if (ret != 0) {
> +		RTE_LOG(DEBUG, EAL, "Failed to init mutex. ret = %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	mutex->mutex_id = m;
> +	m = NULL;
> +
> +cleanup:
> +	free(m);
> +	return ret;
> +}
> +
> +int
> +rte_thread_mutex_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_lock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_unlock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_unlock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_destroy(rte_thread_mutex *mutex)
> +{
> +	int ret = 0;
> +	RTE_VERIFY(mutex != NULL);
> +
> +	ret = pthread_mutex_destroy((pthread_mutex_t *)mutex->mutex_id);
> +	if (ret != 0)
> +		RTE_LOG(DEBUG, EAL, "Unable to destroy mutex, ret = %d\n", ret);
> +
> +	free(mutex->mutex_id);
> +	mutex->mutex_id = NULL;
> +
> +	return ret;
> +}
> +
>  int
>  rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
>  {
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 7c84e32988..09a5fd8add 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -54,6 +54,25 @@ typedef struct {
> 
>  #endif /* RTE_HAS_CPUSET */
> 
> +#define RTE_DECLARE_MUTEX(private_lock)          rte_thread_mutex private_lock
> +
> +#define RTE_DEFINE_MUTEX(private_lock)\
> +RTE_INIT(__rte_ ## private_lock ## _init)\
> +{\
> +	RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
> +}
> +
> +#define RTE_INIT_MUTEX(private_lock)\
> +static RTE_DECLARE_MUTEX(private_lock);\
> +RTE_DEFINE_MUTEX(private_lock)

Hmm, but that way we can't use  RTE_INIT_MUTEX()
within functions, right?

> +
> +/**
> + * Thread mutex representation.
> + */
> +typedef struct rte_thread_mutex_tag {
> +	void *mutex_id;  /**< mutex identifier */
> +} rte_thread_mutex;

I wonder can't we have something like that instead:

for posix:
typedef pthread_mutex_t rte_thread_mutex_t;
for windows: 
typedef struct rte_thread_mutex {
	int initialized;
	CRITICAL_SECTION cs;
}  rte_thread_mutex_t;

Then for posix:
#define RTE_INIT_MUTEX(mx) do {\
	*(mx) = PTHREAD_MUTEX_INITIALIZER; \
} while(0)

#define RTE_DESTROY_MUTEX(mx)	do {} while (0); /*empty */

For windows:
#define RTE_INIT_MUTEX(mx) do {\
	If ((mx)->initialized == 0) {
		InitializeCriticalSection((mx)->cs);
		(mx)->initialized = 1;
	}
} while (0)

#define RTE_DESTROY_MUTEX(mx)	do {
	if ((mx)->initialized != 0) { \
		DeleteCriticalSection((mx)->cs);
	}
} while (0)

That way we'll keep ability for static initialization on posix systems,
and would avoid need to allocate actual mutex object from the heap. 

>  /**
>   * Returned by rte_thread_barrier_wait() when call is successful.
>   */
> @@ -314,6 +333,72 @@ __rte_experimental
>  int rte_thread_set_priority(rte_thread_t thread_id,
>  		enum rte_thread_priority priority);
> 
> +/**
> + * Initializes a mutex.
> + *
> + * @param mutex
> + *    The mutex to be initialized.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_init(rte_thread_mutex *mutex);
> +
> +/**
> + * Locks a mutex.
> + *
> + * @param mutex
> + *    The mutex to be locked.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_lock(rte_thread_mutex *mutex);
> +
> +/**
> + * Unlocks a mutex.
> + *
> + * @param mutex
> + *    The mutex to be unlocked.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_unlock(rte_thread_mutex *mutex);
> +
> +/**
> + * Tries to lock a mutex.If the mutex is already held by a different thread,
> + * the function returns without blocking.
> + *
> + * @param mutex
> + *    The mutex that will be acquired, if not already locked.
> + *
> + * @return
> + *   On success, if the mutex is acquired, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_try_lock(rte_thread_mutex *mutex);
> +
> +/**
> + * Releases all resources associated with a mutex.
> + *
> + * @param mutex
> + *    The mutex to be uninitialized.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_destroy(rte_thread_mutex *mutex);
> +
>  /**
>   * Initializes a synchronization barrier.
>   *