From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id A33F858F4 for ; Mon, 9 Feb 2015 14:12:38 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 09 Feb 2015 05:12:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,543,1418112000"; d="scan'208";a="683075917" Received: from pgsmsx101.gar.corp.intel.com ([10.221.44.78]) by orsmga002.jf.intel.com with ESMTP; 09 Feb 2015 05:12:36 -0800 Received: from kmsmsx154.gar.corp.intel.com (172.21.73.14) by PGSMSX101.gar.corp.intel.com (10.221.44.78) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 9 Feb 2015 21:12:35 +0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by KMSMSX154.gar.corp.intel.com (172.21.73.14) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 9 Feb 2015 21:12:34 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.62]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.46]) with mapi id 14.03.0195.001; Mon, 9 Feb 2015 21:12:34 +0800 From: "Liang, Cunming" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for common thread API Thread-Index: AQHQPoxkCcxjkTcYs0q2zE+79mvOm5zmsHwAgAGf7sA= Date: Mon, 9 Feb 2015 13:12:34 +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-7-git-send-email-cunming.liang@intel.com> <54D7C04C.70308@6wind.com> In-Reply-To: <54D7C04C.70308@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 06/17] eal: add eal_common_thread.c for common thread API 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:12:39 -0000 > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Monday, February 09, 2015 4:00 AM > To: Liang, Cunming; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for > common thread API >=20 > Hi, >=20 > On 02/02/2015 03:02 AM, Cunming Liang wrote: > > The API works for both EAL thread and none EAL thread. > > When calling rte_thread_set_affinity, the *_socket_id* and > > *_cpuset* of calling thread will be updated if the thread > > successful set the cpu affinity. > > > > [...] > > +int > > +rte_thread_set_affinity(rte_cpuset_t *cpusetp) > > +{ > > + int s; > > + unsigned lcore_id; > > + pthread_t tid; > > + > > + if (!cpusetp) > > + return -1; >=20 > Is it really needed to test that cpusetp is not NULL? [LCM] Accept, we can ignore it and depend on pthread_setaffinity_np() to re= turn failure. >=20 > > + > > + lcore_id =3D rte_lcore_id(); > > + if (lcore_id !=3D (unsigned)LCORE_ID_ANY) { >=20 > This is strange to see something that cannot happen: > lcore_id =3D=3D LCORE_ID_ANY is only possible after your patch is 12/17 > is added. Maybe it can be reordered to avoid this inconsistency? [LCM] You're right, here do some re-order. The point is to make everything ready before switching the default value to= -1. And we can have the whole function implement in one patch. It just won't take effect, but won't bring additional risk. >=20 > > + /* EAL thread */ > > + tid =3D lcore_config[lcore_id].thread_id; > > + > > + s =3D pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp); > > + if (s !=3D 0) { > > + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); > > + return -1; > > + } > > + > > + /* store socket_id in TLS for quick access */ > > + RTE_PER_LCORE(_socket_id) =3D > > + eal_cpuset_socket_id(cpusetp); > > + > > + /* store cpuset in TLS for quick access */ > > + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp, > > + sizeof(rte_cpuset_t)); > > + > > + /* update lcore_config */ > > + lcore_config[lcore_id].socket_id =3D RTE_PER_LCORE(_socket_id); > > + rte_memcpy(&lcore_config[lcore_id].cpuset, cpusetp, > > + sizeof(rte_cpuset_t)); > > + } else { > > + /* none EAL thread */ > > + tid =3D pthread_self(); > > + > > + s =3D pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp); > > + if (s !=3D 0) { > > + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); > > + return -1; > > + } > > + > > + /* store cpuset in TLS for quick access */ > > + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp, > > + sizeof(rte_cpuset_t)); > > + > > + /* store socket_id in TLS for quick access */ > > + RTE_PER_LCORE(_socket_id) =3D > > + eal_cpuset_socket_id(cpusetp); > > + } >=20 > Why not always using pthread_self() to get the tid? [LCM] Good point, I haven't notice it. >=20 > I think most of the code could be factorized here. The only difference > (which is hard to see as is as code is not exactly ordered in the same > manner) is that the config is updated in case it's an EAL thread. [LCM] Accept. >=20 >=20 >=20 > > + > > + return 0; > > +} > > + > > +int > > +rte_thread_get_affinity(rte_cpuset_t *cpusetp) > > +{ > > + if (!cpusetp) > > + return -1; >=20 > Same here. This is the only reason why rte_thread_get_affinity() could > fail. Removing this test would allow to change the API to return void > instead. It will avoid a useless test below in > eal_thread_dump_affinity(). [LCM] The cpusetp is used as destination of memcpy and the function suppose= an EAL API. I don't think it's a good idea to remove the check, do you ? >=20 > > + > > + rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset), > > + sizeof(rte_cpuset_t)); > > + > > + return 0; > > +} > > + > > +void > > +eal_thread_dump_affinity(char str[], unsigned size) > > +{ > > + rte_cpuset_t cpuset; > > + unsigned cpu; > > + int ret; > > + unsigned int out =3D 0; > > + > > + if (rte_thread_get_affinity(&cpuset) < 0) { > > + str[0] =3D '\0'; > > + return; > > + } >=20 > This one could be removed it the (=3D=3D NULL) test is removed. >=20 > > + > > + for (cpu =3D 0; cpu < RTE_MAX_LCORE; cpu++) { > > + if (!CPU_ISSET(cpu, &cpuset)) > > + continue; > > + > > + ret =3D snprintf(str + out, > > + size - out, "%u,", cpu); > > + if (ret < 0 || (unsigned)ret >=3D size - out) > > + break; >=20 > On the contrary, I think here returning an error to the user > would be useful so he can knows that the dump is not complete. [LCM] accept. >=20 >=20 > Regards, > Olivier