Re: [PATCH v4 05/17] eal: new TLS definition and API declaration

-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
Sent: Monday, February 09, 2015 4:00 AM
To: Liang, Cunming; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API
declaration

Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:

quoted

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.

[LCM] Accept.

quoted

Signed-off-by: Cunming Liang <redacted>
---
 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

+++++++++++++++++++++++++++--

quoted

 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

quoted

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

quoted

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().

[LCM] Accept.

What is the purpose of CPU_STR_LEN?

[LCM] For default quick reference for str[] definition used in dump_affinity()

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?

[LCM] Yes, always have a '\0' in the end.

This should be described in the API comments.

[LCM] Accept.

Maybe adding a return
value could help the user to determine if the string was truncated.

[LCM] Good idea, so the user can continue to print '...' for the truncated part.

quoted

+
+
 #endif /* EAL_THREAD_H */

diff --git a/lib/librte_eal/common/include/rte_lcore.h

b/lib/librte_eal/common/include/rte_lcore.h

quoted

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".

*/

quoted

+RTE_DECLARE_PER_LCORE(unsigned, _socket_id); /**< Per thread "socket id".

*/

quoted

+RTE_DECLARE_PER_LCORE(rte_cpuset_t, _cpuset); /**< Per thread "cpuset".

*/

quoted

 /**
  * 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.

[LCM] The value initializes as SOCKET_ID_ANY when RTE_DEFINE_PER_LCORE().
And updated in eal_thread_set_affinity() for EAL thread and rte_thread_set_affinity() for non-EAL thread.

Regards,
Olivier