Re: [PATCH v18 1/8] eal: add basic threading functions

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

> Use a portable, type-safe representation for the thread identifier.
> Add functions for comparing thread ids and obtaining the thread id
> for the current thread.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/eal/common/meson.build            |  1 +
>  lib/eal/{unix => common}/rte_thread.c | 57 ++++++++++++++++----------
>  lib/eal/include/rte_thread.h          | 53 ++++++++++++++++++------
>  lib/eal/unix/meson.build              |  1 -
>  lib/eal/version.map                   |  3 ++
>  lib/eal/windows/rte_thread.c          | 58 +++++++++++++++++----------
>  6 files changed, 117 insertions(+), 56 deletions(-)
>  rename lib/eal/{unix => common}/rte_thread.c (66%)
> 
> diff --git a/lib/eal/common/meson.build b/lib/eal/common/meson.build
> index 917758cc65..6bdc9cd854 100644
> --- a/lib/eal/common/meson.build
> +++ b/lib/eal/common/meson.build
> @@ -52,5 +52,6 @@ if not is_windows
>              'hotplug_mp.c',
>              'malloc_mp.c',
>              'rte_keepalive.c',
> +            'rte_thread.c'
>      )
>  endif
> diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/common/rte_thread.c
> similarity index 66%
> rename from lib/eal/unix/rte_thread.c
> rename to lib/eal/common/rte_thread.c
> index c72d619ec1..92a7451b0a 100644
> --- a/lib/eal/unix/rte_thread.c
> +++ b/lib/eal/common/rte_thread.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright 2021 Mellanox Technologies, Ltd
> + * Copyright(c) 2021 Microsoft Corporation
>   */
> 
>  #include <errno.h>
> @@ -16,25 +17,41 @@ struct eal_tls_key {
>  	pthread_key_t thread_index;
>  };
> 
> +rte_thread_t
> +rte_thread_self(void)
> +{
> +	rte_thread_t thread_id;
> +
> +	thread_id.opaque_id = (uintptr_t)pthread_self();
> +
> +	return thread_id;
> +}
> +
> +int
> +rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
> +{
> +	return pthread_equal((pthread_t)t1.opaque_id, (pthread_t)t2.opaque_id);
> +}
> +
>  int
>  rte_thread_key_create(rte_thread_key *key, void (*destructor)(void *))
>  {
>  	int err;
> +	rte_thread_key k;

I know that 'rte_thread_key' wasn't introduced in this patchset,
but wonder - why not 'rte_thread_key_t' as we do for other DPDK typedefs?
 
> -	*key = malloc(sizeof(**key));
> -	if ((*key) == NULL) {
> +	k = malloc(sizeof(*k));
> +	if (k == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> -		rte_errno = ENOMEM;
> -		return -1;
> +		return EINVAL;
>  	}
> -	err = pthread_key_create(&((*key)->thread_index), destructor);
> -	if (err) {
> +	err = pthread_key_create(&(k->thread_index), destructor);
> +	if (err != 0) {
>  		RTE_LOG(DEBUG, EAL, "pthread_key_create failed: %s\n",
>  			 strerror(err));
> -		free(*key);
> -		rte_errno = ENOEXEC;
> -		return -1;
> +		free(k);
> +		return err;
>  	}
> +	*key = k;
>  	return 0;
>  }
> 
> @@ -43,18 +60,16 @@ rte_thread_key_delete(rte_thread_key key)
>  {
>  	int err;
> 
> -	if (!key) {
> +	if (key == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
> -		rte_errno = EINVAL;
> -		return -1;
> +		return EINVAL;
>  	}
>  	err = pthread_key_delete(key->thread_index);
> -	if (err) {
> +	if (err != 0) {
>  		RTE_LOG(DEBUG, EAL, "pthread_key_delete failed: %s\n",
>  			 strerror(err));
>  		free(key);
> -		rte_errno = ENOEXEC;
> -		return -1;
> +		return err;
>  	}
>  	free(key);
>  	return 0;
> @@ -65,17 +80,15 @@ rte_thread_value_set(rte_thread_key key, const void *value)
>  {
>  	int err;
> 
> -	if (!key) {
> +	if (key == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
> -		rte_errno = EINVAL;
> -		return -1;
> +		return EINVAL;
>  	}
>  	err = pthread_setspecific(key->thread_index, value);
> -	if (err) {
> +	if (err != 0) {
>  		RTE_LOG(DEBUG, EAL, "pthread_setspecific failed: %s\n",
>  			strerror(err));
> -		rte_errno = ENOEXEC;
> -		return -1;
> +		return err;
>  	}
>  	return 0;
>  }
> @@ -83,7 +96,7 @@ rte_thread_value_set(rte_thread_key key, const void *value)
>  void *
>  rte_thread_value_get(rte_thread_key key)
>  {
> -	if (!key) {
> +	if (key == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
>  		rte_errno = EINVAL;
>  		return NULL;
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 8be8ed8f36..c9cdeb07aa 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -1,6 +1,8 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2021 Mellanox Technologies, Ltd
> + * Copyright(c) 2021 Microsoft Corporation
>   */
> +#include <stdint.h>

Do we really need it in common .h?
I don't see anything sched.h that we start to use with that patch.

>  #include <rte_os.h>
>  #include <rte_compat.h>
> @@ -20,11 +22,45 @@
>  extern "C" {
>  #endif
> 
> +#include <sched.h>
> +
> +/**
> + * Thread id descriptor.
> + */
> +typedef struct rte_thread_tag {
> +	uintptr_t opaque_id; /**< thread identifier */
> +} rte_thread_t;
> +
>  /**
>   * TLS key type, an opaque pointer.
>   */
>  typedef struct eal_tls_key *rte_thread_key;
> 
> +/**
> + * Get the id of the calling thread.
> + *
> + * @return
> + *   Return the thread id of the calling thread.
> + */
> +__rte_experimental
> +rte_thread_t rte_thread_self(void);
> +
> +/**
> + * Check if 2 thread ids are equal.
> + *
> + * @param t1
> + *   First thread id.
> + *
> + * @param t2
> + *   Second thread id.
> + *
> + * @return
> + *   If the ids are equal, return nonzero.
> + *   Otherwise, return 0.
> + */
> +__rte_experimental
> +int rte_thread_equal(rte_thread_t t1, rte_thread_t t2);
> +
>  #ifdef RTE_HAS_CPUSET
> 
>  /**
> @@ -63,9 +99,7 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
>   *
>   * @return
>   *   On success, zero.
> - *   On failure, a negative number and an error number is set in rte_errno.
> - *   rte_errno can be: ENOMEM  - Memory allocation error.
> - *                     ENOEXEC - Specific OS error.
> + *   On failure, return a positive errno-style error number.
>   */
> 
>  __rte_experimental
> @@ -80,9 +114,7 @@ int rte_thread_key_create(rte_thread_key *key,
>   *
>   * @return
>   *   On success, zero.
> - *   On failure, a negative number and an error number is set in rte_errno.
> - *   rte_errno can be: EINVAL  - Invalid parameter passed.
> - *                     ENOEXEC - Specific OS error.
> + *   On failure, return a positive errno-style error number.
>   */
>  __rte_experimental
>  int rte_thread_key_delete(rte_thread_key key);
> @@ -97,9 +129,7 @@ int rte_thread_key_delete(rte_thread_key key);
>   *
>   * @return
>   *   On success, zero.
> - *   On failure, a negative number and an error number is set in rte_errno.
> - *   rte_errno can be: EINVAL  - Invalid parameter passed.
> - *                     ENOEXEC - Specific OS error.
> + *   On failure, return a positive errno-style error number.
>   */
>  __rte_experimental
>  int rte_thread_value_set(rte_thread_key key, const void *value);
> @@ -112,9 +142,8 @@ int rte_thread_value_set(rte_thread_key key, const void *value);
>   *
>   * @return
>   *   On success, value data pointer (can also be NULL).
> - *   On failure, NULL and an error number is set in rte_errno.
> - *   rte_errno can be: EINVAL  - Invalid parameter passed.
> - *                     ENOEXEC - Specific OS error.
> + *   On failure, NULL and a positive error number is set in rte_errno.
> + *
>   */
>  __rte_experimental
>  void *rte_thread_value_get(rte_thread_key key);
> diff --git a/lib/eal/unix/meson.build b/lib/eal/unix/meson.build
> index e3ecd3e956..cb6d233721 100644
> --- a/lib/eal/unix/meson.build
> +++ b/lib/eal/unix/meson.build
> @@ -6,5 +6,4 @@ sources += files(
>          'eal_unix_memory.c',
>          'eal_unix_timer.c',
>          'eal_firmware.c',
> -        'rte_thread.c',
>  )