From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id C64FB5927 for ; Tue, 10 Feb 2015 03:54:13 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 09 Feb 2015 18:54:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,547,1418112000"; d="scan'208";a="683425969" Received: from pgsmsx106.gar.corp.intel.com ([10.221.44.98]) by orsmga002.jf.intel.com with ESMTP; 09 Feb 2015 18:54:11 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by PGSMSX106.gar.corp.intel.com (10.221.44.98) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 10 Feb 2015 10:51:31 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.62]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.197]) with mapi id 14.03.0195.001; Tue, 10 Feb 2015 10:51:30 +0800 From: "Liang, Cunming" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread by assigned cpuset Thread-Index: AQHQPoxpa9Gv0BZYTkqDnan7btr8aJzmsJ+AgAGoScD//8HSgIABIDxw Date: Tue, 10 Feb 2015 02:51:30 +0000 Message-ID: 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> <54D8F02B.20400@6wind.com> In-Reply-To: <54D8F02B.20400@6wind.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Tue, 10 Feb 2015 02:54:14 -0000 > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Tuesday, February 10, 2015 1:37 AM > To: Liang, Cunming; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL threa= d by > assigned cpuset >=20 > Hi, >=20 > 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 th= read 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 =3D 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=3D%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=3D%p;cpuset=3D[%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 lcor= e > >>> * 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 hav= e > linuxapp and freebsdapp in different init sequence. > > I means in linux it init_master before dev_init(), but in freebsd it re= verse. >=20 >=20 > I agree that's something we should fix. >=20 >=20 > > And as the default value of TLS already changes, if dev_init() first an= d 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 ch= ange > follows linuxapp sequence. >=20 > 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" [LCM] Agree. >=20 >=20 >=20 > >> > >> > >>> 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 =3D CPU_ALLOC(RTE_MAX_LCORE); > >>> - if (cpusetp =3D=3D NULL) { > >>> - RTE_LOG(ERR, EAL, "CPU_ALLOC failed\n"); > >>> - return -1; > >>> - } > >>> - > >>> - size =3D CPU_ALLOC_SIZE(RTE_MAX_LCORE); > >>> - CPU_ZERO_S(size, cpusetp); > >>> - CPU_SET_S(rte_lcore_id(), size, cpusetp); > >>> + unsigned lcore_id =3D rte_lcore_id(); > >>> > >>> thread =3D pthread_self(); > >>> - s =3D pthread_setaffinity_np(thread, size, cpusetp); > >>> + s =3D pthread_setaffinity_np(thread, sizeof(cpuset_t), > >>> + &lcore_config[lcore_id].cpuset); > >>> if (s !=3D 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() woul= d > >> 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. >=20 > I think the mail was properly sent, you can see it here: > http://dpdk.org/ml/archives/dev/2015-February/012556.html >=20 > 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. [LCM] Thanks, agree your point, will update on v5. >=20 >=20 > >>> + > >>> + /* store socket_id in TLS for quick access */ > >>> + RTE_PER_LCORE(_socket_id) =3D > >>> + 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 =3D RTE_PER_LCORE(_socket_id); > >>> > >>> - thread =3D pthread_self(); > >>> - s =3D pthread_setaffinity_np(thread, sizeof( cpuset ), &cpuset); > >>> - if (s !=3D 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(), b= ut 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. >=20 > 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. [LCM] Ok, thanks. >=20 >=20 >=20