From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <harry.van.haaren@intel.com>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
by dpdk.org (Postfix) with ESMTP id DCB661B1B1
for <dev@dpdk.org>; Thu, 19 Oct 2017 12:02:16 +0200 (CEST)
Received: from fmsmga004.fm.intel.com ([10.253.24.48])
by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
19 Oct 2017 03:02:15 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.43,400,1503385200"; d="scan'208";a="325085755"
Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99])
by fmsmga004.fm.intel.com with ESMTP; 19 Oct 2017 03:02:15 -0700
Received: from irsmsx102.ger.corp.intel.com ([169.254.2.180]) by
IRSMSX107.ger.corp.intel.com ([169.254.10.239]) with mapi id 14.03.0319.002;
Thu, 19 Oct 2017 11:02:14 +0100
From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: "Eads, Gage" <gage.eads@intel.com>
CC: "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: Re: [dpdk-dev] [PATCH] example: add new service cores sample
application
Thread-Index: AdM2GOP/29a/eCBhQF++AnNGuzNSfgSoi4sg
Date: Thu, 19 Oct 2017 10:02:14 +0000
Message-ID: <E923DB57A917B54B9182A2E928D00FA650FC796E@IRSMSX102.ger.corp.intel.com>
References: <9184057F7FC11744A2107296B6B8EB1E13FF25E2@fmsmsx101.amr.corp.intel.com>
In-Reply-To: <9184057F7FC11744A2107296B6B8EB1E13FF25E2@fmsmsx101.amr.corp.intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODcwMzkwZjAtOGE4Ny00ZWUzLWE1OWQtMzU3YzVlZmU3ODMzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlZ2dzR0WWFXV2grVm9QWlZ5aFZESFNYV1gzVTl1UzNScmswYUJhcUpXQTA9In0=
x-ctpclassification: CTP_IC
dlp-product: dlpe-windows
dlp-version: 11.0.0.116
dlp-reaction: no-action
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH] example: add new service cores sample
application
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <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: Thu, 19 Oct 2017 10:02:17 -0000
> From: Eads, Gage=20
> Sent: Monday, September 25, 2017 5:32 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] example: add new service cores sample app=
lication
>=20
> Neat example. Looks good overall, I just have a few questions.
>=20
> > +#define PROFILE_SERVICE_PER_CORE 8
>=20
> Any reason not to use 5 here? That would remove a few zeroes from the pro=
files[] initializers.
Nope - good suggestion, Fixed in v2.
> > +static int
> > +apply_profile(int profile_id)
> > +{
> > + uint32_t i;
> > + uint32_t s;
> > + int ret;
> > + struct profile *p =3D &profiles[profile_id];
> > + const uint8_t core_off =3D 1;
> > +
> > + for (i =3D 0; i < p->num_cores; i++) {
> > + ret =3D rte_service_lcore_add(i + core_off);
> > + if (ret && ret !=3D -EALREADY)
> > + printf("core %d added ret %d\n=
", i + core_off, ret);
>=20
> I'm wondering if this and the other printfs in this function should be rt=
e_panics? These seem like fatal errors.
They're not fatal, they shouldn't (and don't) occur in the sample app. The =
error handling is there to be a good example - panic() doesn't seem a good =
suggestion to handle and error.. I'll leave as is.
>=20
> > +
> > + ret =3D rte_service_lcore_start(i + core_off)=
;
> > + if (ret && ret !=3D -EALREADY)
> > + printf("core %d start ret %d\n=
", i + core_off, ret);
> > +
> > + for (s =3D 0; s < NUM_SERVICES; s++) {
> > + if (rte_service_map_lcore_set(=
s, i + core_off,
> > + =
p->cores[i].mapped_services[s]))
> > + rte_panic("fail=
ed to map lcore to 1\n");
>=20
> What does '1' refer to here? Perhaps it should be: rte_panic("failed to m=
ap lcore %d to %s\n", i + core_off, services[s].name);
Good catch, fixed in v2.