From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 9E09A2BD3 for ; 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" To: "Pattan, Reshma" , "dev@dpdk.org" CC: "Wiles, Keith" , "Mcnamara, John" , "Byrne, Stephen1" , "Tamboli, Amit S" , "Padubidri, Sanjay A" , "Patel, Amol" , "Kovacevic, Marko" 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="us-ascii" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Mar 2019 10:22:11 -0000 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 ] --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 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 | device_id=3D)," > > "(queue=3D)," > > "(rx-dev=3D |" > > @@ -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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id C491FA05D3 for ; 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 ; 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" To: "Pattan, Reshma" , "dev@dpdk.org" CC: "Wiles, Keith" , "Mcnamara, John" , "Byrne, Stephen1" , "Tamboli, Amit S" , "Padubidri, Sanjay A" , "Patel, Amol" , "Kovacevic, Marko" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 ] --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 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 | device_id=3D)," > > "(queue=3D)," > > "(rx-dev=3D |" > > @@ -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