[dpdk-dev,v4,05/17] eal: new TLS definition and API declaration
Commit Message
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
Signed-off-by: Cunming Liang <cunming.liang@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(-)
Comments
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@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
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev@dpdk.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:
> > 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.
>
> >
> > Signed-off-by: Cunming Liang <cunming.liang@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().
[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.
>
> > +
> > +
> > #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.
[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
Hi,
On 02/09/2015 01:45 PM, Liang, Cunming wrote:
>>> +/**
>>> + * 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()
So the API comment of the function is not placed at the right
place.
A comment "Default buffer size to use with eal_thread_dump_affinity()"
should be added above CPU_STR_LEN. Also, it could be renamed in
RTE_CPU_STR_LEN or RTE_CPU_AFFINITY_STR_LEN.
>>> @@ -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.
> [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.
This is done in a later patches:
"eal: set _lcore_id and _socket_id to (-1) by default"
"eal: apply affinity of EAL thread by assigned cpuset"
That's why I said there is probably an issue with the ordering
of the patches as these values are used here but initialized
later in the series.
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 10, 2015 1:26 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API
> declaration
>
> Hi,
>
> On 02/09/2015 01:45 PM, Liang, Cunming wrote:
> >>> +/**
> >>> + * 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()
>
> So the API comment of the function is not placed at the right
> place.
>
> A comment "Default buffer size to use with eal_thread_dump_affinity()"
> should be added above CPU_STR_LEN. Also, it could be renamed in
> RTE_CPU_STR_LEN or RTE_CPU_AFFINITY_STR_LEN.
[LCM] Got you.
>
>
>
> >>> @@ -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.
> > [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.
>
> This is done in a later patches:
>
> "eal: set _lcore_id and _socket_id to (-1) by default"
> "eal: apply affinity of EAL thread by assigned cpuset"
>
> That's why I said there is probably an issue with the ordering
> of the patches as these values are used here but initialized
> later in the series.
[LCM] Will reorder them in next version.
@@ -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
@@ -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);
+
+
#endif /* EAL_THREAD_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);
}
/**
@@ -229,6 +232,28 @@ rte_get_next_lcore(unsigned i, int skip_master, int wrap)
i<RTE_MAX_LCORE; \
i = rte_get_next_lcore(i, 1, 0))
+/**
+ * Set core affinity of the current thread.
+ * Support both EAL and none-EAL thread and update TLS.
+ *
+ * @param cpusetp
+ * Point to cpu_set_t for setting current thread affinity.
+ * @return
+ * On success, return 0; otherwise return -1;
+ */
+int rte_thread_set_affinity(rte_cpuset_t *cpusetp);
+
+/**
+ * Get core affinity of the current thread.
+ *
+ * @param cpusetp
+ * Point to cpu_set_t for getting current thread cpu affinity.
+ * @return
+ * On success, return 0; otherwise return -1;
+ */
+int rte_thread_get_affinity(rte_cpuset_t *cpusetp);
+
+
#ifdef __cplusplus
}
#endif
@@ -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