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 712EF58F4 for ; Mon, 9 Feb 2015 18:17:04 +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 1YKs0s-0006jq-M5; Mon, 09 Feb 2015 18:20:52 +0100 Message-ID: <54D8EB85.40809@6wind.com> Date: Mon, 09 Feb 2015 18:16:53 +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-5-git-send-email-cunming.liang@intel.com> <54D7C042.6060104@6wind.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 04/17] eal: add support parsing socket_id from cpuset 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:17:04 -0000 Hi, On 02/09/2015 01:26 PM, Liang, Cunming wrote: >>> @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void >> *arg); >>> */ >>> void eal_thread_init_master(unsigned lcore_id); >>> >>> +/** >>> + * Get the NUMA socket id from cpu id. >>> + * This function is private to EAL. >>> + * >>> + * @param cpu_id >>> + * The logical process id. >>> + * @return >>> + * socket_id or SOCKET_ID_ANY >>> + */ >>> +unsigned eal_cpu_socket_id(unsigned cpu_id); >> >> Wouldn't it be better to rename the existing function cpu_socket_id() >> in eal_cpu_socket_id() and export it in eal_thread.h? >> >> In case of bsd where cpu_socket_id() is implemented using a #define, >> a new function should be created returning 0. > [LCM] In eal_lcore.c, the cpu_socket_id()/cpu_core_id() defined as static and only used in rte_eal_cpu_init(). > I suppose the purpose of origin design is to make the sysfs parsing only visible in the file. > No matter remove the 'static' prefix of cpu_core_id() or add a new wrap eal_cpu_socket_id(), it results in a new extern EAL API. > So I prefer not change the visibility of the origin static function but have one as extern interface. Yes, but I don't see what is the advantage of using a wrapper. If there is no advantage, I think the one with the less code is better. >>> +static inline int >>> +eal_cpuset_socket_id(rte_cpuset_t *cpusetp) >>> +{ >>> + unsigned cpu = 0; >>> + int socket_id = SOCKET_ID_ANY; >>> + int sid; >>> + >>> + if (cpusetp == NULL) >>> + return SOCKET_ID_ANY; >>> + >>> + do { >>> + if (!CPU_ISSET(cpu, cpusetp)) >>> + continue; >>> + >>> + if (socket_id == SOCKET_ID_ANY) >>> + socket_id = eal_cpu_socket_id(cpu); >>> + >>> + sid = eal_cpu_socket_id(cpu); >>> + if (socket_id != sid) { >>> + socket_id = SOCKET_ID_ANY; >>> + break; >>> + } >>> + >>> + } while (++cpu < RTE_MAX_LCORE); >>> + >>> + return socket_id; >>> +} >> >> >> I don't think this function should be inlined. >> >> As this function is not used, it could be interesting for reviewers >> to understand when > [LCM] It's used in eal_thread_set_affinity() of eal_thread.c. As it's not visible in the patch, could you add an explanation in the commit log?