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
