From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH v4 05/17] eal: new TLS definition and API declaration Date: Sun, 08 Feb 2015 21:00:05 +0100 Message-ID: <54D7C045.4070701@6wind.com> References: <1422491072-5114-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-6-git-send-email-cunming.liang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit To: Cunming Liang , dev-VfR2kkLFssw@public.gmane.org Return-path: In-Reply-To: <1422842559-13617-6-git-send-email-cunming.liang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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 > --- > 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 > #include > #include > +#include > > #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