From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 914545A43 for ; Sun, 8 Feb 2015 21:00:24 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YKY5G-0005PL-Ly; Sun, 08 Feb 2015 21:04:11 +0100 Message-ID: <54D7C045.4070701@6wind.com> Date: Sun, 08 Feb 2015 21:00:05 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 MIME-Version: 1.0 To: Cunming Liang , dev@dpdk.org 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> In-Reply-To: <1422842559-13617-6-git-send-email-cunming.liang@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API declaration X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Feb 2015 20:00:24 -0000 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