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 7363158F4 for ; Mon, 9 Feb 2015 18:36:59 +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 1YKsK5-0006mn-IA; Mon, 09 Feb 2015 18:40:46 +0100 Message-ID: <54D8F02B.20400@6wind.com> Date: Mon, 09 Feb 2015 18:36:43 +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-9-git-send-email-cunming.liang@intel.com> <54D7C069.4020900@6wind.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread by assigned 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:36:59 -0000 Hi, On 02/09/2015 02:48 PM, Liang, Cunming wrote: >> -----Original Message----- >> From: Olivier MATZ [mailto:olivier.matz@6wind.com] >> Sent: Monday, February 09, 2015 4:01 AM >> To: Liang, Cunming; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread by >> assigned cpuset >> >> Hi, >> >> On 02/02/2015 03:02 AM, Cunming Liang wrote: >>> EAL threads use assigned cpuset to set core affinity during startup. >>> It keeps 1:1 mapping, if no '--lcores' option is used. >>> >>> [...] >>> >>> lib/librte_eal/bsdapp/eal/eal.c | 13 ++++--- >>> lib/librte_eal/bsdapp/eal/eal_thread.c | 63 +++++++++--------------------- >>> lib/librte_eal/linuxapp/eal/eal.c | 7 +++- >>> lib/librte_eal/linuxapp/eal/eal_thread.c | 67 +++++++++++--------------------- >>> 4 files changed, 54 insertions(+), 96 deletions(-) >>> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c >>> index 69f3c03..98c5a83 100644 >>> --- a/lib/librte_eal/bsdapp/eal/eal.c >>> +++ b/lib/librte_eal/bsdapp/eal/eal.c >>> @@ -432,6 +432,7 @@ rte_eal_init(int argc, char **argv) >>> int i, fctret, ret; >>> pthread_t thread_id; >>> static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0); >>> + char cpuset[CPU_STR_LEN]; >>> >>> if (!rte_atomic32_test_and_set(&run_once)) >>> return -1; >>> @@ -502,13 +503,17 @@ rte_eal_init(int argc, char **argv) >>> if (rte_eal_pci_init() < 0) >>> rte_panic("Cannot init PCI\n"); >>> >>> - RTE_LOG(DEBUG, EAL, "Master core %u is ready (tid=%p)\n", >>> - rte_config.master_lcore, thread_id); >>> - >>> eal_check_mem_on_local_socket(); >>> >>> rte_eal_mcfg_complete(); >>> >>> + eal_thread_init_master(rte_config.master_lcore); >>> + >>> + eal_thread_dump_affinity(cpuset, CPU_STR_LEN); >>> + >>> + RTE_LOG(DEBUG, EAL, "Master lcore %u is ready (tid=%p;cpuset=[%s])\n", >>> + rte_config.master_lcore, thread_id, cpuset); >>> + >>> if (rte_eal_dev_init() < 0) >>> rte_panic("Cannot init pmd devices\n"); >>> >>> @@ -532,8 +537,6 @@ rte_eal_init(int argc, char **argv) >>> rte_panic("Cannot create thread\n"); >>> } >>> >>> - eal_thread_init_master(rte_config.master_lcore); >>> - >>> /* >>> * Launch a dummy function on all slave lcores, so that master lcore >>> * knows they are all ready when this function returns. >> >> I wonder if changing this may have an impact on third-party drivers >> that already use a management thread. Before the patch, the init() >> function of the external library was called with default affinities, >> and now it's called with the affinity from master lcore. >> >> I think it should at least be noticed in the commit log. >> >> Why are you doing this change? (I don't say it's a bad change, but >> I don't understand why you are doing it here) > [LCM] To be honest, the main purpose is I don't found any reason to have linuxapp and freebsdapp in different init sequence. > I means in linux it init_master before dev_init(), but in freebsd it reverse. I agree that's something we should fix. > And as the default value of TLS already changes, if dev_init() first and using those TLS, the result will be not in an EAL thread. > But actually they're in the EAL master thread. So I prefer to do the change follows linuxapp sequence. That makes sense. Is it possible to have this reordering in a separate patch? The title could be "eal: standardize init sequence between linux and bsd" >> >> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c >> b/lib/librte_eal/bsdapp/eal/eal_thread.c >>> index d0c077b..5b16302 100644 >>> --- a/lib/librte_eal/bsdapp/eal/eal_thread.c >>> +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c >>> @@ -103,55 +103,27 @@ eal_thread_set_affinity(void) >>> { >>> int s; >>> pthread_t thread; >>> - >>> -/* >>> - * According to the section VERSIONS of the CPU_ALLOC man page: >>> - * >>> - * The CPU_ZERO(), CPU_SET(), CPU_CLR(), and CPU_ISSET() macros were >> added >>> - * in glibc 2.3.3. >>> - * >>> - * CPU_COUNT() first appeared in glibc 2.6. >>> - * >>> - * CPU_AND(), CPU_OR(), CPU_XOR(), CPU_EQUAL(), >> CPU_ALLOC(), >>> - * CPU_ALLOC_SIZE(), CPU_FREE(), CPU_ZERO_S(), CPU_SET_S(), >> CPU_CLR_S(), >>> - * CPU_ISSET_S(), CPU_AND_S(), CPU_OR_S(), CPU_XOR_S(), and >> CPU_EQUAL_S() >>> - * first appeared in glibc 2.7. >>> - */ >>> -#if defined(CPU_ALLOC) >>> - size_t size; >>> - cpu_set_t *cpusetp; >>> - >>> - cpusetp = CPU_ALLOC(RTE_MAX_LCORE); >>> - if (cpusetp == NULL) { >>> - RTE_LOG(ERR, EAL, "CPU_ALLOC failed\n"); >>> - return -1; >>> - } >>> - >>> - size = CPU_ALLOC_SIZE(RTE_MAX_LCORE); >>> - CPU_ZERO_S(size, cpusetp); >>> - CPU_SET_S(rte_lcore_id(), size, cpusetp); >>> + unsigned lcore_id = rte_lcore_id(); >>> >>> thread = pthread_self(); >>> - s = pthread_setaffinity_np(thread, size, cpusetp); >>> + s = pthread_setaffinity_np(thread, sizeof(cpuset_t), >>> + &lcore_config[lcore_id].cpuset); >>> if (s != 0) { >>> RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); >>> - CPU_FREE(cpusetp); >>> return -1; >>> } >>> >>> - CPU_FREE(cpusetp); >>> -#else /* CPU_ALLOC */ >>> - cpuset_t cpuset; >>> - CPU_ZERO( &cpuset ); >>> - CPU_SET( rte_lcore_id(), &cpuset ); >>> + /* acquire system unique id */ >>> + rte_gettid(); >> >> As suggested in the previous patch, I think having rte_init_tid() would >> be clearer here. > [LCM] Sorry, I didn't get your [PATCH v4 07/17] comments, probably the mailbox issue. > Do you suggest to have a rte_init_tid() but not do syscall on the first time ? > Any benefit, rte_gettid() looks like more simple and straight forward. I think the mail was properly sent, you can see it here: http://dpdk.org/ml/archives/dev/2015-February/012556.html Usually, "get" functions return a value and have no side effects. "init" functions return nothing (or an error code) but have a side effect which is to initialize an internal state. >>> + >>> + /* store socket_id in TLS for quick access */ >>> + RTE_PER_LCORE(_socket_id) = >>> + eal_cpuset_socket_id(&lcore_config[lcore_id].cpuset); >>> + >>> + CPU_COPY(&lcore_config[lcore_id].cpuset, &RTE_PER_LCORE(_cpuset)); >>> + >>> + lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id); >>> >>> - thread = pthread_self(); >>> - s = pthread_setaffinity_np(thread, sizeof( cpuset ), &cpuset); >>> - if (s != 0) { >>> - RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); >>> - return -1; >>> - } >>> -#endif >> >> You are removing a lot of code that was using CPU_ALLOC(). >> Are we sure that the cpuset_t type is large enough to store all the >> CPUs? >> >> It looks the current value of CPU_SETSIZE is 1024 now, but I wonder >> if this code was written when this value was lower. Could you check if >> it can happen today (maybe with an old libc)? A problem can occur if >> the size of cpuset_t is lower that the size of RTE_MAX_LCORE. > [LCM] I found actually the MACRO is not just for support CPU_ALLOC(), but for linux or freebsd. > In freebsdapp, there's no CPU_ALLOC defined, it use fixed width *cpuset_t*. > In linuxapp, there's CPU_ALLOC defined, it use cpu_set_t* and dynamic CPU_ALLOC(RTE_MAX_LCORE). > But actually RTE_MAX_LCORE < 1024(sizeof(cpu_set_t)). > After using rte_cpuset_t, there's no additional reason to use CPU_ALLOC only for linuxapp and choose a small but dynamic width. I did a quick search on google, and it seems CPU_SETSIZE is 1024 for a long time. So you are right, there is probably no reason to keep CPU_ALLOC(). As I said in a previous mail, it could be useful in the future when the number of CPUs will reach 1024, but we have some time to handle this.