From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <cunming.liang@intel.com>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
by dpdk.org (Postfix) with ESMTP id A33F858F4
for <dev@dpdk.org>; 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" <cunming.liang@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <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: <D0158A423229094DA7ABF71CF2FA0DA3118D7DF6@shsmsx102.ccr.corp.intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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