[dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API declaration

[Olivier MATZ]

Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> 1. add two TLS *_socket_id* and *_cpuset*
> 2. add two external API rte_thread_set/get_affinity
> 3. add one internal API eal_thread_dump_affinity

To me, it's a bit strage to add an API withtout the associated code.
Maybe you have a good reason to do that, but I think in this case it
should be explained in the commit log.

> 
> Signed-off-by: Cunming Liang <cunming.liang at intel.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal_thread.c    |  2 ++
>  lib/librte_eal/common/eal_thread.h        | 14 ++++++++++++++
>  lib/librte_eal/common/include/rte_lcore.h | 29 +++++++++++++++++++++++++++--
>  lib/librte_eal/linuxapp/eal/eal_thread.c  |  2 ++
>  4 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c b/lib/librte_eal/bsdapp/eal/eal_thread.c
> index ab05368..10220c7 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> @@ -56,6 +56,8 @@
>  #include "eal_thread.h"
>  
>  RTE_DEFINE_PER_LCORE(unsigned, _lcore_id);
> +RTE_DEFINE_PER_LCORE(unsigned, _socket_id);
> +RTE_DEFINE_PER_LCORE(rte_cpuset_t, _cpuset);
>  
>  /*
>   * Send a message to a slave lcore identified by slave_id to call a
> diff --git a/lib/librte_eal/common/eal_thread.h b/lib/librte_eal/common/eal_thread.h
> index a25ee86..28edf51 100644
> --- a/lib/librte_eal/common/eal_thread.h
> +++ b/lib/librte_eal/common/eal_thread.h
> @@ -102,4 +102,18 @@ eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
>  	return socket_id;
>  }
>  
> +/**
> + * Dump the current pthread cpuset.
> + * This function is private to EAL.
> + *
> + * @param str
> + *   The string buffer the cpuset will dump to.
> + * @param size
> + *   The string buffer size.
> + */
> +#define CPU_STR_LEN            256
> +void
> +eal_thread_dump_affinity(char str[], unsigned size);

Although it's equivalent for function arguments, I think "char *str" is
usually preferred over "char str[]". See for instance in snprintf() or
fgets().

What is the purpose of CPU_STR_LEN?

What occurs if the size of the dump is greater than the size of the
given buffer? Is the string truncated? Is there a \0 at the end?
This should be described in the API comments. Maybe adding a return
value could help the user to determine if the string was truncated.

> +
> +
>  #endif /* EAL_THREAD_H */
> diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
> index 4c7d6bb..facdbdc 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -43,6 +43,7 @@
>  #include <rte_per_lcore.h>
>  #include <rte_eal.h>
>  #include <rte_launch.h>
> +#include <rte_memory.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -80,7 +81,9 @@ struct lcore_config {
>   */
>  extern struct lcore_config lcore_config[RTE_MAX_LCORE];
>  
> -RTE_DECLARE_PER_LCORE(unsigned, _lcore_id); /**< Per core "core id". */
> +RTE_DECLARE_PER_LCORE(unsigned, _lcore_id);  /**< Per thread "lcore id". */
> +RTE_DECLARE_PER_LCORE(unsigned, _socket_id); /**< Per thread "socket id". */
> +RTE_DECLARE_PER_LCORE(rte_cpuset_t, _cpuset); /**< Per thread "cpuset". */
>  
>  /**
>   * Return the ID of the execution unit we are running on.
> @@ -146,7 +149,7 @@ rte_lcore_index(int lcore_id)
>  static inline unsigned
>  rte_socket_id(void)
>  {
> -	return lcore_config[rte_lcore_id()].socket_id;
> +	return RTE_PER_LCORE(_socket_id);
>  }

I don't see where the _socket_id variable is assigned. I think there
is probably an issue with the splitting of the patches.

Regards,
Olivier