From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by dpdk.space (Postfix) with ESMTP id C491FA05D3
for <public@inbox.dpdk.org>; Fri, 29 Mar 2019 11:22:13 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id D8F942BF4;
Fri, 29 Mar 2019 11:22:12 +0100 (CET)
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
by dpdk.org (Postfix) with ESMTP id 9E09A2BD3
for <dev@dpdk.org>; Fri, 29 Mar 2019 11:22:10 +0100 (CET)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga001.jf.intel.com ([10.7.209.18])
by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
29 Mar 2019 03:22:09 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,284,1549958400"; d="scan'208";a="218715746"
Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204])
by orsmga001.jf.intel.com with ESMTP; 29 Mar 2019 03:22:09 -0700
Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by
FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS)
id 14.3.408.0; Fri, 29 Mar 2019 03:22:09 -0700
Received: from bgsmsx109.gar.corp.intel.com (10.223.4.211) by
fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS)
id 14.3.408.0; Fri, 29 Mar 2019 03:22:08 -0700
Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.171]) by
BGSMSX109.gar.corp.intel.com ([169.254.10.43]) with mapi id 14.03.0415.000;
Fri, 29 Mar 2019 15:52:04 +0530
From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: "Pattan, Reshma" <reshma.pattan@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
CC: "Wiles, Keith" <keith.wiles@intel.com>, "Mcnamara, John"
<john.mcnamara@intel.com>, "Byrne, Stephen1" <stephen1.byrne@intel.com>,
"Tamboli, Amit S" <amit.tamboli@intel.com>, "Padubidri, Sanjay A"
<sanjay.padubidri@intel.com>, "Patel, Amol" <amol.patel@intel.com>,
"Kovacevic, Marko" <marko.kovacevic@intel.com>
Thread-Topic: [PATCH v3] app/pdump: enhance to support multi-core capture
Thread-Index: AQHU5Xd+Lb2ce9wwwkqHWxMSPoma4aYiB5cAgABcYyA=
Date: Fri, 29 Mar 2019 10:22:04 +0000
Message-ID:
<4C9E0AB70F954A408CC4ADDBF0F8FA7D4D32780A@BGSMSX101.gar.corp.intel.com>
References: <20190328145746.11786-1-vipin.varghese@intel.com>
<20190328150406.12051-1-vipin.varghese@intel.com>
<3AEA2BF9852C6F48A459DA490692831F2A432BA2@irsmsx110.ger.corp.intel.com>
In-Reply-To: <3AEA2BF9852C6F48A459DA490692831F2A432BA2@irsmsx110.ger.corp.intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-ctpclassification: CTP_NT
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTVmMjIyMDEtZmI3Yy00YWYyLWI2YzQtYTEwMDk3ZTM5NzliIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibjVIdmRUU1FUT1BmZ1J4XC9mZkFrMk1CMngzaWZFbUFZZWE0eHYweFIxVUJWaWZmYytqczhSK1dpYThKWXJJekwifQ==
dlp-product: dlpe-windows
dlp-version: 11.0.400.15
dlp-reaction: no-action
x-originating-ip: [10.223.10.10]
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v3] app/pdump: enhance to support multi-core
capture
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190329102204.7YcbsrsoLqgFBMOrOAPjvuFxKN9N8ztyGp2e7yclHUI@z>
Hi Reshma,
snipped
> >
> > /* true if x is a power of 2 */
> > #define POWEROF2(x) ((((x)-1) & (x)) =3D=3D 0) @@ -144,7 +145,7 @@ sta=
tic
> > volatile uint8_t quit_signal; static void pdump_usage(const char *prg=
name) {
> > - printf("usage: %s [EAL options] -- --pdump "
> > + printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "
>=20
> Using -l option same as eal is confusing. Please use other name.
Current implementation passes core-mask '-cx1' as EAL argument. The check f=
or user argument '-l <core1,core2,core3' is done before rte_eal_init. Once =
identified it is replaced with c_flag.
Hence I disagree to the point it is confusing.
> Also how about moving this new option inside --pdump"" so it will be cle=
arly
> known that the particular core will be associated to that tuple.
Yes, this can be done.
>=20
> Also, I have some major concern, check my below comments.
Thanks for your concerns, let me try to address them below.
>=20
> > "'(port=3D<port id> | device_id=3D<pci id or vdev name>),"
> > "(queue=3D<queue_id>),"
> > "(rx-dev=3D<iface or pcap file> |"
> > @@ -415,6 +416,7 @@ print_pdump_stats(void)
> > for (i =3D 0; i < num_tuples; i++) {
> > printf("##### PDUMP DEBUG STATS #####\n");
> > pt =3D &pdump_t[i];
> > + printf(" =3D=3D DPDK interface (%d) =3D=3D\n", i);
>=20
> Here good to print the portid/deviceid and queue info, instead of printin=
g pdump
> tuple index i? User might not understand that.
I am not sure, why you mention that I am displaying tuple index with I here=
?
> Use ### instead of =3D=3D=3D as above.
I can do this, but is there specific reasoning for "####" as it is used to =
represent main header?
>=20
> > +
> > static inline void
> > dump_packets(void)
> > {
> > int i;
> > - struct pdump_tuples *pt;
> > + uint32_t lcore_id =3D 0;
> > +
> > + lcore_id =3D rte_get_next_lcore(lcore_id, 1, 1);
> > +
> > + if (rte_lcore_count() =3D=3D 1) {
> > + while (!quit_signal) {
> > + for (i =3D 0; i < num_tuples; i++) {
> > + struct pdump_tuples *pt =3D &pdump_t[i];
> > + pdump_packets(pt);
> > + }
> > + }
> > + } else {
> > + printf(" Tuples (%u) lcores (%u)\n",
> > + num_tuples, rte_lcore_count());
> > +
> > + if ((uint32_t)num_tuples >=3D rte_lcore_count()) {
> > + printf("Insufficent Cores\n");
> Typo %s/Insufficent/
Ok
>=20
>=20
> > + for (i =3D 0; i < argc; i++) {
> > + if (strstr(argv[i], "-l")) {
> > + snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
>=20
> You are taking this as application arguments then making it as eal argume=
nt to
> run the application.
I have explained the same above.
> Why not enable the needed number of cores in core mask using eal options =
-l=20
I think what you are saying is "allow user to pass -l option or -c option b=
efore `--`". Then before invoking rte_eal_init replace it. Is this your req=
uirement?
and
> have new core param in pdump tuple to run that tuple on that core.
>=20
> Ex:
> If you check l3fwd as an example the cores should enabled using -c or -l =
and then
> they have separate --config l3fwd option in which they specify the core o=
n which
> the packet processing should be run. Please check that and similar would =
be good
> here too.
I have already explained, pdump application makes static assignment of '-cx=
1'. If you try passing '-c' or '-l' the error check in rte_eal_init will pr=
event such assignment.
>=20
> > + strlcpy(argv[i], "", 2);
> > + strlcpy(argv[i + 1], "", 2);
>=20
> Why is this?
I have explained this above.
Anyway, rte_strlcpy should be used instead of strlcpy.
Ok
>=20
> Thanks,
> Reshma
Hi Reshma, thanks for feedbacks on cosmetic, spelling and using rte_strlcpy