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 F00B958E8 for ; Mon, 9 Feb 2015 18:26:11 +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 1YKs9i-0006mA-2o; Mon, 09 Feb 2015 18:29:59 +0100 Message-ID: <54D8EDA8.7030303@6wind.com> Date: Mon, 09 Feb 2015 18:26:00 +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: "Liang, Cunming" , "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> <54D7C045.4070701@6wind.com> In-Reply-To: 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: Mon, 09 Feb 2015 17:26:12 -0000 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.