From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 2C63D2C7A for ; Mon, 9 Feb 2015 14:48:28 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 09 Feb 2015 05:41:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,544,1418112000"; d="scan'208";a="524762853" Received: from pgsmsx107.gar.corp.intel.com ([10.221.44.105]) by orsmga003.jf.intel.com with ESMTP; 09 Feb 2015 05:40:25 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.110.15) by PGSMSX107.gar.corp.intel.com (10.221.44.105) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 9 Feb 2015 21:48:23 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.62]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.161]) with mapi id 14.03.0195.001; Mon, 9 Feb 2015 21:48:18 +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+AgAGoScA= Date: Mon, 9 Feb 2015 13:48:17 +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> In-Reply-To: <54D7C069.4020900@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: Mon, 09 Feb 2015 13:48:30 -0000 > -----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 threa= d by > assigned cpuset >=20 > Hi, >=20 > 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/ea= l/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 lcore > > * knows they are all ready when this function returns. >=20 > 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. >=20 > I think it should at least be noticed in the commit log. >=20 > 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 li= nuxapp and freebsdapp in different init sequence. I means in linux it init_master before dev_init(), but in freebsd it revers= e. And as the default value of TLS already changes, if dev_init() first and us= ing 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. >=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(); >=20 > 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 mail= box issue. Do you suggest to have a rte_init_tid() but not do syscall on the first tim= e ? Any benefit, rte_gettid() looks like more simple and straight forward.=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 >=20 > 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? >=20 > 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 f= or 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_A= LLOC(RTE_MAX_LCORE). But actually RTE_MAX_LCORE < 1024(sizeof(cpu_set_t)).=20 After using rte_cpuset_t, there's no additional reason to use CPU_ALLOC onl= y for linuxapp and choose a small but dynamic width. >=20 >=20 > Regards, > Olivier