[PATCH] app/pdump: enhance to support multi-core capture

[PATCH] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Enhance pdump application, to allow user to run on multiple cores.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c           | 73 ++++++++++++++++++++++++++++++++------
 doc/guides/tools/pdump.rst |  5 +++
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..110fd59d9 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -62,6 +62,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define COREMASK_SIZE 32
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -144,7 +145,7 @@ static volatile uint8_t quit_signal;
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options] -- [-c=<coremask in HEX>] --pdump "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -415,6 +416,7 @@ print_pdump_stats(void)
 	for (i = 0; i < num_tuples; i++) {
 		printf("##### PDUMP DEBUG STATS #####\n");
 		pt = &pdump_t[i];
+		printf(" == DPDK interface (%d) ==\n", i);
 		printf(" -packets dequeued:			%"PRIu64"\n",
 							pt->stats.dequeue_pkts);
 		printf(" -packets transmitted to vdev:		%"PRIu64"\n",
@@ -834,22 +836,62 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	lcore_id = rte_get_next_lcore(lcore_id, 0, 1);
+
+	if (rte_lcore_count() == 1) {
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++) {
+				struct pdump_tuples *pt = &pdump_t[i];
+				pdump_packets(pt);
+			}
+		}
+	} else {
+		printf(" Tuples (%u) lcores (%u)\n",
+			num_tuples, rte_lcore_count());
+
+		if ((uint32_t)num_tuples >= rte_lcore_count()) {
+			printf("Insufficent Cores\n");
+			return;
+		}
 
-	while (!quit_signal) {
 		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+			/* use remote launch for n interfaces */
+			rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+			lcore_id = rte_get_next_lcore(lcore_id, 0, 1);
+
+			if (rte_eal_wait_lcore(lcore_id) < 0)
+				rte_exit(EXIT_FAILURE, "failed to wait\n");
 		}
+
+		while (!quit_signal)
+			;
 	}
 }
 
@@ -860,7 +902,7 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
+	char c_flag[COREMASK_SIZE] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 3];
@@ -868,6 +910,15 @@ main(int argc, char **argv)
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
+	for (i = 0; i < argc; i++) {
+		if (strstr(argv[i], "-c")) {
+			snprintf(c_flag, RTE_DIM(c_flag), "-c %s", argv[i+1]);
+			strlcpy(argv[i], "", 2);
+			strlcpy(argv[i + 1], "", 2);
+			break;
+		}
+	}
+
 	argp[0] = argv[0];
 	argp[1] = c_flag;
 	argp[2] = n_flag;
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..c7be6d0c7 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [-c <core-mask in HEX>]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,9 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``-c`` command line option is optional and HEX representation of core-mask
+allows to run capture for each ``--pdump`` to run on lcore.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -113,3 +117,4 @@ Example
 .. code-block:: console
 
    $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -- -c 0x700 --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap' --pdump 'port=1,queue=*,tx-dev=/tmp/tx.pcap'
-- 
2.17.1

[PATCH v2] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Enhance pdump application, to allow user to run on multiple cores.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c           | 75 ++++++++++++++++++++++++++++++++------
 doc/guides/tools/pdump.rst |  5 +++
 2 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..0d0e80ff1 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -62,6 +62,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define CORES_STR_SIZE 32
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -144,7 +145,7 @@ static volatile uint8_t quit_signal;
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options] -- [-l=<list of cores>] --pdump "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -415,6 +416,7 @@ print_pdump_stats(void)
 	for (i = 0; i < num_tuples; i++) {
 		printf("##### PDUMP DEBUG STATS #####\n");
 		pt = &pdump_t[i];
+		printf(" == DPDK interface (%d) ==\n", i);
 		printf(" -packets dequeued:			%"PRIu64"\n",
 							pt->stats.dequeue_pkts);
 		printf(" -packets transmitted to vdev:		%"PRIu64"\n",
@@ -834,22 +836,62 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
+
+	if (rte_lcore_count() == 1) {
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++) {
+				struct pdump_tuples *pt = &pdump_t[i];
+				pdump_packets(pt);
+			}
+		}
+	} else {
+		printf(" Tuples (%u) lcores (%u)\n",
+			num_tuples, rte_lcore_count());
+
+		if ((uint32_t)num_tuples >= rte_lcore_count()) {
+			printf("Insufficent Cores\n");
+			return;
+		}
 
-	while (!quit_signal) {
 		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+			/* use remote launch for n interfaces */
+			rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+			lcore_id = rte_get_next_lcore(lcore_id, 0, 1);
+
+			if (rte_eal_wait_lcore(lcore_id) < 0)
+				rte_exit(EXIT_FAILURE, "failed to wait\n");
 		}
+
+		while (!quit_signal)
+			;
 	}
 }
 
@@ -860,7 +902,7 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
+	char c_flag[CORES_STR_SIZE] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 3];
@@ -868,6 +910,17 @@ main(int argc, char **argv)
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
+	for (i = 0; i < argc; i++) {
+		if (strstr(argv[i], "-l")) {
+			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
+			strlcpy(argv[i], "", 2);
+			strlcpy(argv[i + 1], "", 2);
+			break;
+		}
+	}
+
+	printf(" c_flag %s", c_flag);
+
 	argp[0] = argv[0];
 	argp[1] = c_flag;
 	argp[2] = n_flag;
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..3cfd9fc44 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [-l <list of cores>]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,9 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``-l`` command line option is optional and it takes list cores on which
+capture for each ``--pdump`` is to be run.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -113,3 +117,4 @@ Example
 .. code-block:: console
 
    $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -- -l 3,4,5 --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap' --pdump 'port=1,queue=*,tx-dev=/tmp/tx.pcap'
-- 
2.17.1

[PATCH v3] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Enhance pdump application, to allow user to run on multiple cores.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

Change Log:

V3:
 - correct the parse_usage - Vipin Varghese
 - add change log - Vipin Varghese

V2:
 - Replace option '-c' to '-l' - Keith Wiles
---
 app/pdump/main.c           | 75 ++++++++++++++++++++++++++++++++------
 doc/guides/tools/pdump.rst |  5 +++
 2 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..a2e092420 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -62,6 +62,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define CORES_STR_SIZE 32
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -144,7 +145,7 @@ static volatile uint8_t quit_signal;
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -415,6 +416,7 @@ print_pdump_stats(void)
 	for (i = 0; i < num_tuples; i++) {
 		printf("##### PDUMP DEBUG STATS #####\n");
 		pt = &pdump_t[i];
+		printf(" == DPDK interface (%d) ==\n", i);
 		printf(" -packets dequeued:			%"PRIu64"\n",
 							pt->stats.dequeue_pkts);
 		printf(" -packets transmitted to vdev:		%"PRIu64"\n",
@@ -834,22 +836,62 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
+
+	if (rte_lcore_count() == 1) {
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++) {
+				struct pdump_tuples *pt = &pdump_t[i];
+				pdump_packets(pt);
+			}
+		}
+	} else {
+		printf(" Tuples (%u) lcores (%u)\n",
+			num_tuples, rte_lcore_count());
+
+		if ((uint32_t)num_tuples >= rte_lcore_count()) {
+			printf("Insufficent Cores\n");
+			return;
+		}
 
-	while (!quit_signal) {
 		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+			/* use remote launch for n interfaces */
+			rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+			lcore_id = rte_get_next_lcore(lcore_id, 0, 1);
+
+			if (rte_eal_wait_lcore(lcore_id) < 0)
+				rte_exit(EXIT_FAILURE, "failed to wait\n");
 		}
+
+		while (!quit_signal)
+			;
 	}
 }
 
@@ -860,7 +902,7 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
+	char c_flag[CORES_STR_SIZE] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 3];
@@ -868,6 +910,17 @@ main(int argc, char **argv)
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
+	for (i = 0; i < argc; i++) {
+		if (strstr(argv[i], "-l")) {
+			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
+			strlcpy(argv[i], "", 2);
+			strlcpy(argv[i + 1], "", 2);
+			break;
+		}
+	}
+
+	printf(" c_flag %s", c_flag);
+
 	argp[0] = argv[0];
 	argp[1] = c_flag;
 	argp[2] = n_flag;
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..3cfd9fc44 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [-l <list of cores>]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,9 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``-l`` command line option is optional and it takes list cores on which
+capture for each ``--pdump`` is to be run.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -113,3 +117,4 @@ Example
 .. code-block:: console
 
    $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -- -l 3,4,5 --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap' --pdump 'port=1,queue=*,tx-dev=/tmp/tx.pcap'
-- 
2.17.1

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Wiles, Keith]

> On Mar 28, 2019, at 10:04 AM, Varghese, Vipin <vipin.varghese@intel.com> wrote:
> 
> Enhance pdump application, to allow user to run on multiple cores.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> 
> Change Log:
> 
> V3:
> - correct the parse_usage - Vipin Varghese
> - add change log - Vipin Varghese
> 
> V2:
> - Replace option '-c' to '-l' - Keith Wiles
> ---
> app/pdump/main.c           | 75 ++++++++++++++++++++++++++++++++------
> doc/guides/tools/pdump.rst |  5 +++
> 2 files changed, 69 insertions(+), 11 deletions(-)
> 

Reviewed-by: Keith Wiles <keith.wiles*intel.com>

Regards,
Keith

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> 
>  /* true if x is a power of 2 */
>  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -144,7 +145,7 @@ static volatile
> uint8_t quit_signal;  static void  pdump_usage(const char *prgname)  {
> -	printf("usage: %s [EAL options] -- --pdump "
> +	printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "

Using -l option same as eal is confusing. Please use other name.
Also how about moving this  new option inside --pdump"" so it will be clearly known that the particular core will be associated to that tuple.

Also, I have some major concern, check my below comments.

>  			"'(port=<port id> | device_id=<pci id or vdev name>),"
>  			"(queue=<queue_id>),"
>  			"(rx-dev=<iface or pcap file> |"
> @@ -415,6 +416,7 @@ print_pdump_stats(void)
>  	for (i = 0; i < num_tuples; i++) {
>  		printf("##### PDUMP DEBUG STATS #####\n");
>  		pt = &pdump_t[i];
> +		printf(" == DPDK interface (%d) ==\n", i);

Here good to print the portid/deviceid and queue info, instead of printing pdump tuple index  i? User might not understand that.
Use ### instead of === as above.
 
> +
>  static inline void
>  dump_packets(void)
>  {
>  	int i;
> -	struct pdump_tuples *pt;
> +	uint32_t lcore_id = 0;
> +
> +	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
> +
> +	if (rte_lcore_count() == 1) {
> +		while (!quit_signal) {
> +			for (i = 0; i < num_tuples; i++) {
> +				struct pdump_tuples *pt = &pdump_t[i];
> +				pdump_packets(pt);
> +			}
> +		}
> +	} else {
> +		printf(" Tuples (%u) lcores (%u)\n",
> +			num_tuples, rte_lcore_count());
> +
> +		if ((uint32_t)num_tuples >= rte_lcore_count()) {
> +			printf("Insufficent Cores\n");
Typo %s/Insufficent/


> +	for (i = 0; i < argc; i++) {
> +		if (strstr(argv[i], "-l")) {
> +			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);

You are taking this as application arguments then making it as eal argument  to run the application.  
Why not enable the needed number of cores in core mask using eal options -l and have new core param in pdump tuple to run that tuple on that core.

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 on which the packet processing should be run. Please check that and similar would be good here too.

> +			strlcpy(argv[i], "", 2);
> +			strlcpy(argv[i + 1], "", 2);

Why is this? Anyway, rte_strlcpy should be used instead of strlcpy.

Thanks,
Reshma

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi Reshma,

snipped
> >
> >  /* true if x is a power of 2 */
> >  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -144,7 +145,7 @@ static
> > volatile uint8_t quit_signal;  static void  pdump_usage(const char *prgname)  {
> > -	printf("usage: %s [EAL options] -- --pdump "
> > +	printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "
> 
> Using -l option same as eal is confusing. Please use other name.
Current implementation passes core-mask '-cx1' as EAL argument. The check for 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 clearly
> known that the particular core will be associated to that tuple.
Yes, this can be done.

> 
> Also, I have some major concern, check my below comments.
Thanks for your concerns, let me try to address them below.

> 
> >  			"'(port=<port id> | device_id=<pci id or vdev name>),"
> >  			"(queue=<queue_id>),"
> >  			"(rx-dev=<iface or pcap file> |"
> > @@ -415,6 +416,7 @@ print_pdump_stats(void)
> >  	for (i = 0; i < num_tuples; i++) {
> >  		printf("##### PDUMP DEBUG STATS #####\n");
> >  		pt = &pdump_t[i];
> > +		printf(" == DPDK interface (%d) ==\n", i);
> 
> Here good to print the portid/deviceid and queue info, instead of printing 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 === as above.
I can do this, but is there specific reasoning for "####" as it is used to represent main header?

> 
> > +
> >  static inline void
> >  dump_packets(void)
> >  {
> >  	int i;
> > -	struct pdump_tuples *pt;
> > +	uint32_t lcore_id = 0;
> > +
> > +	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
> > +
> > +	if (rte_lcore_count() == 1) {
> > +		while (!quit_signal) {
> > +			for (i = 0; i < num_tuples; i++) {
> > +				struct pdump_tuples *pt = &pdump_t[i];
> > +				pdump_packets(pt);
> > +			}
> > +		}
> > +	} else {
> > +		printf(" Tuples (%u) lcores (%u)\n",
> > +			num_tuples, rte_lcore_count());
> > +
> > +		if ((uint32_t)num_tuples >= rte_lcore_count()) {
> > +			printf("Insufficent Cores\n");
> Typo %s/Insufficent/
Ok

> 
> 
> > +	for (i = 0; i < argc; i++) {
> > +		if (strstr(argv[i], "-l")) {
> > +			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
> 
> You are taking this as application arguments then making it as eal argument  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 
I think what you are saying is "allow user to pass -l option or -c option before `--`". Then before invoking rte_eal_init replace it. Is this your requirement?

and
> have new core param in pdump tuple to run that tuple on that core.
> 
> 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 on 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 '-cx1'. If you try passing '-c' or '-l' the error check in rte_eal_init will prevent such assignment.

> 
> > +			strlcpy(argv[i], "", 2);
> > +			strlcpy(argv[i + 1], "", 2);
> 
> Why is this?
I have explained this above.


 Anyway, rte_strlcpy should be used instead of strlcpy.
Ok

> 
> Thanks,
> Reshma
Hi Reshma, thanks for feedbacks on cosmetic, spelling and using rte_strlcpy

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Subject: RE: [PATCH v3] app/pdump: enhance to support multi-core capture
> >
> > >  			"'(port=<port id> | device_id=<pci id or vdev name>),"
> > >  			"(queue=<queue_id>),"
> > >  			"(rx-dev=<iface or pcap file> |"
> > > @@ -415,6 +416,7 @@ print_pdump_stats(void)
> > >  	for (i = 0; i < num_tuples; i++) {
> > >  		printf("##### PDUMP DEBUG STATS #####\n");
> > >  		pt = &pdump_t[i];
> > > +		printf(" == DPDK interface (%d) ==\n", i);
> >
> > Here good to print the portid/deviceid and queue info, instead of
> > printing 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?
> 

What is i here?
i is the index used in for loop to iterate through the pdump_tuples array.
So printing i doesn't make sense, instead port and queue info are good 
options to print if you want to have this log.

> > Use ### instead of === as above.
> I can do this, but is there specific reasoning for "####" as it is used to represent
> main header?

to follow consistency with  existing display of ###.

> > Why not enable the needed number of cores in core mask using eal
> > options -l
> I think what you are saying is "allow user to pass -l option or -c option before `--

Yes, and remove existing static -c1 method.
So cores that should be enabled will come from eal options, then in --pdump  you 
have new core param which will be used to launch packet dump function for that pdump tuple.
While parsing application pdump core param, you should check this core is enabled in coremask of eal. That is the way how other applications do.

> `". Then before invoking rte_eal_init replace it. Is this your requirement?
> 

After new suggestion of removing -c1 static approach the above question will not be applicable.

> > 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 on 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 '-cx1'. If
> you try passing '-c' or '-l' the error check in rte_eal_init will prevent such
> assignment.

Now you will remove existing -c1 static assignment and use eal passed -l and -c , so it should be fine now.

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Ferruh Yigit]

On 3/29/2019 10:22 AM, Varghese, Vipin wrote:
> Hi Reshma,
> 
> snipped
>>>
>>>  /* true if x is a power of 2 */
>>>  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -144,7 +145,7 @@ static
>>> volatile uint8_t quit_signal;  static void  pdump_usage(const char *prgname)  {
>>> -	printf("usage: %s [EAL options] -- --pdump "
>>> +	printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "
>>
>> Using -l option same as eal is confusing. Please use other name.
> Current implementation passes core-mask '-cx1' as EAL argument. The check for 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.

I agree with Reshma, if there is a need to run in different cores, lets remove
the hardcoded core information from application and manage the core selection in
eal level, instead of having this in application.

And in app level, you can say which core to use for that specific pdump, overall
something like:

dpdk-pdump -l 20,23 -- --pdump 'port=0,queue=*,core=21,rx-dev=/tmp/rx.pcap'
--pdump 'port=1,queue=*,core=22,tx-dev=/tmp/tx.pcap'


> 
>> Also how about moving this  new option inside --pdump"" so it will be clearly
>> known that the particular core will be associated to that tuple.
> Yes, this can be done.
> 
>>
>> Also, I have some major concern, check my below comments.
> Thanks for your concerns, let me try to address them below.
> 
>>
>>>  			"'(port=<port id> | device_id=<pci id or vdev name>),"
>>>  			"(queue=<queue_id>),"
>>>  			"(rx-dev=<iface or pcap file> |"
>>> @@ -415,6 +416,7 @@ print_pdump_stats(void)
>>>  	for (i = 0; i < num_tuples; i++) {
>>>  		printf("##### PDUMP DEBUG STATS #####\n");
>>>  		pt = &pdump_t[i];
>>> +		printf(" == DPDK interface (%d) ==\n", i);
>>
>> Here good to print the portid/deviceid and queue info, instead of printing 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 === as above.
> I can do this, but is there specific reasoning for "####" as it is used to represent main header?
> 
>>
>>> +
>>>  static inline void
>>>  dump_packets(void)
>>>  {
>>>  	int i;
>>> -	struct pdump_tuples *pt;
>>> +	uint32_t lcore_id = 0;
>>> +
>>> +	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
>>> +
>>> +	if (rte_lcore_count() == 1) {
>>> +		while (!quit_signal) {
>>> +			for (i = 0; i < num_tuples; i++) {
>>> +				struct pdump_tuples *pt = &pdump_t[i];
>>> +				pdump_packets(pt);
>>> +			}
>>> +		}
>>> +	} else {
>>> +		printf(" Tuples (%u) lcores (%u)\n",
>>> +			num_tuples, rte_lcore_count());
>>> +
>>> +		if ((uint32_t)num_tuples >= rte_lcore_count()) {
>>> +			printf("Insufficent Cores\n");
>> Typo %s/Insufficent/
> Ok
> 
>>
>>
>>> +	for (i = 0; i < argc; i++) {
>>> +		if (strstr(argv[i], "-l")) {
>>> +			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
>>
>> You are taking this as application arguments then making it as eal argument  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 
> I think what you are saying is "allow user to pass -l option or -c option before `--`". Then before invoking rte_eal_init replace it. Is this your requirement?
> 
> and
>> have new core param in pdump tuple to run that tuple on that core.
>>
>> 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 on 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 '-cx1'. If you try passing '-c' or '-l' the error check in rte_eal_init will prevent such assignment.
> 
>>
>>> +			strlcpy(argv[i], "", 2);
>>> +			strlcpy(argv[i + 1], "", 2);
>>
>> Why is this?
> I have explained this above.
> 
> 
>  Anyway, rte_strlcpy should be used instead of strlcpy.
> Ok
> 
>>
>> Thanks,
>> Reshma
> Hi Reshma, thanks for feedbacks on cosmetic, spelling and using rte_strlcpy
> 

Re: [PATCH v3] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi Reshma & Ferruh,

Summarizing the discussion with maintainer and proposed changes below.

1. Agreed to make changes for migrating 'strlcpy' to 'rte_strlcpy'.
2. Agreed to make changes for spelling error.
3. Informed it is user decision to enable multiple core capture for queue-pair.
4. Informed master core cannot participate in capture, as it requires book keeping for future enhancements like file size, and packet count.
5. Disagreed to the statement '-l cores' option is confusing as user should have the option to specify the cores.
6. Disagreed to the option suggested from maintainer to enhance '--pdump to add core', as it leads to combinations when option is passed for a few and not for others.
7. Printing port-queue pair instance with core as debug is agreed, but sharing information once capture is stopped does not look useful. But will enhance to do same.
8. In my humble opinion, there should be default core for call back which is core 0. Removing 'c_flag' is not right way after rte_eal_init is not correct. Hence user arguments especially (if pdump is to run on multiple cores) should be checked before rte_eal_init.

Action Item:
1. Send or wait for patch, to remove the core default value.
2. Send v5 for the above agreed points.

Thanks
Vipin Varghese


> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, March 29, 2019 10:33 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; 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>
> Subject: Re: [dpdk-dev] [PATCH v3] app/pdump: enhance to support multi-core
> capture
> 
> On 3/29/2019 10:22 AM, Varghese, Vipin wrote:
> > Hi Reshma,
> >
> > snipped
> >>>
> >>>  /* true if x is a power of 2 */
> >>>  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -144,7 +145,7 @@
> >>> static volatile uint8_t quit_signal;  static void  pdump_usage(const char
> *prgname)  {
> >>> -	printf("usage: %s [EAL options] -- --pdump "
> >>> +	printf("usage: %s [EAL options] -- [-l <list of cores>] --pdump "
> >>
> >> Using -l option same as eal is confusing. Please use other name.
> > Current implementation passes core-mask '-cx1' as EAL argument. The check for
> 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.
> 
> I agree with Reshma, if there is a need to run in different cores, lets remove the
> hardcoded core information from application and manage the core selection in
> eal level, instead of having this in application.
> 
> And in app level, you can say which core to use for that specific pdump, overall
> something like:
> 
> dpdk-pdump -l 20,23 -- --pdump 'port=0,queue=*,core=21,rx-
> dev=/tmp/rx.pcap'
> --pdump 'port=1,queue=*,core=22,tx-dev=/tmp/tx.pcap'
> 
> 
> >
> >> Also how about moving this  new option inside --pdump"" so it will be
> >> clearly known that the particular core will be associated to that tuple.
> > Yes, this can be done.
> >
> >>
> >> Also, I have some major concern, check my below comments.
> > Thanks for your concerns, let me try to address them below.
> >
> >>
> >>>  			"'(port=<port id> | device_id=<pci id or vdev name>),"
> >>>  			"(queue=<queue_id>),"
> >>>  			"(rx-dev=<iface or pcap file> |"
> >>> @@ -415,6 +416,7 @@ print_pdump_stats(void)
> >>>  	for (i = 0; i < num_tuples; i++) {
> >>>  		printf("##### PDUMP DEBUG STATS #####\n");
> >>>  		pt = &pdump_t[i];
> >>> +		printf(" == DPDK interface (%d) ==\n", i);
> >>
> >> Here good to print the portid/deviceid and queue info, instead of
> >> printing 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 === as above.
> > I can do this, but is there specific reasoning for "####" as it is used to represent
> main header?
> >
> >>
> >>> +
> >>>  static inline void
> >>>  dump_packets(void)
> >>>  {
> >>>  	int i;
> >>> -	struct pdump_tuples *pt;
> >>> +	uint32_t lcore_id = 0;
> >>> +
> >>> +	lcore_id = rte_get_next_lcore(lcore_id, 1, 1);
> >>> +
> >>> +	if (rte_lcore_count() == 1) {
> >>> +		while (!quit_signal) {
> >>> +			for (i = 0; i < num_tuples; i++) {
> >>> +				struct pdump_tuples *pt = &pdump_t[i];
> >>> +				pdump_packets(pt);
> >>> +			}
> >>> +		}
> >>> +	} else {
> >>> +		printf(" Tuples (%u) lcores (%u)\n",
> >>> +			num_tuples, rte_lcore_count());
> >>> +
> >>> +		if ((uint32_t)num_tuples >= rte_lcore_count()) {
> >>> +			printf("Insufficent Cores\n");
> >> Typo %s/Insufficent/
> > Ok
> >
> >>
> >>
> >>> +	for (i = 0; i < argc; i++) {
> >>> +		if (strstr(argv[i], "-l")) {
> >>> +			snprintf(c_flag, RTE_DIM(c_flag), "-l %s", argv[i+1]);
> >>
> >> You are taking this as application arguments then making it as eal
> >> argument  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
> > I think what you are saying is "allow user to pass -l option or -c option before `--
> `". Then before invoking rte_eal_init replace it. Is this your requirement?
> >
> > and
> >> have new core param in pdump tuple to run that tuple on that core.
> >>
> >> 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 on 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 '-cx1'. If
> you try passing '-c' or '-l' the error check in rte_eal_init will prevent such
> assignment.
> >
> >>
> >>> +			strlcpy(argv[i], "", 2);
> >>> +			strlcpy(argv[i + 1], "", 2);
> >>
> >> Why is this?
> > I have explained this above.
> >
> >
> >  Anyway, rte_strlcpy should be used instead of strlcpy.
> > Ok
> >
> >>
> >> Thanks,
> >> Reshma
> > Hi Reshma, thanks for feedbacks on cosmetic, spelling and using
> > rte_strlcpy
> >

[PATCH v4 0/2] app/pdump: enhance to support unique cores

[Vipin Varghese]

The patch series enhances application to support pdump capture to run
on unique cores.

Motivation
==========

DPDK pdump capture tool currently runs as secondary on the default core 0.
For all --pdump, core 0 iterates and capture packets. This leads to drops
and delay in the capture. This introduces skew in result and performance
bottleneck.

In order to allow --pdump to unique cores, the pdump application has to be
support multi-core in EAL arguments and each --pdump has to be mapped to
unique cores.

Status
======

 - Default c_flag option is removed.
 - with option --multi, the application can run on unique cores.

Change Log:
===========

V4:
 - spelling correction - Reshma Pathan
 - remove c_falg - Reshma Pathan

V3:
 - correct the parse_usage - Vipin Varghese
 - add change log - Vipin Varghese

V2:
 - Replace option '-c' to '-l' - Keith Wiles

Vipin Varghese (2):
  app/pdump: remove core restriction
  app/pdump: enhance to support multi-core capture

 app/pdump/main.c           | 90 ++++++++++++++++++++++++++++++--------
 doc/guides/tools/pdump.rst |  8 +++-
 2 files changed, 78 insertions(+), 20 deletions(-)

Future:
=======
 - In --multi mode, allow master to stop --pdump for desired packets.
 - In --multi mode, allow master to stop --pdump for desired file size.

-- 
2.17.1

[PATCH v4 1/2] app/pdump: remove core restriction

[Vipin Varghese]

PDUMP application is being limited to run on default first core.
The patch removes the restriction, allowing the user to run on any of
selected cores in EAL args. If no args are passed, the logic runs on
default master core.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..c1db2eb8d 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -860,23 +860,21 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
-	char *argp[argc + 3];
+	char *argp[argc + 2];
 
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
 	argp[0] = argv[0];
-	argp[1] = c_flag;
-	argp[2] = n_flag;
-	argp[3] = mp_flag;
+	argp[1] = n_flag;
+	argp[2] = mp_flag;
 
 	for (i = 1; i < argc; i++)
-		argp[i + 3] = argv[i];
+		argp[i + 2] = argv[i];
 
-	argc += 3;
+	argc += 2;
 
 	diag = rte_eal_init(argc, argp);
 	if (diag < 0)
@@ -886,7 +884,7 @@ main(int argc, char **argv)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
 
 	argc -= diag;
-	argv += (diag - 3);
+	argv += (diag - 2);
 
 	/* parse app arguments */
 	if (argc > 1) {
-- 
2.17.1

[PATCH v4 2/2] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Add option --multi, to enhance the pdump application to allow capture
on unique cores for each --pdump option. If option --multi is ignored
the default capture occurs on a single core for all --pdump options.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c           | 76 ++++++++++++++++++++++++++++++++------
 doc/guides/tools/pdump.rst |  8 +++-
 2 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c1db2eb8d..997c8942f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -28,6 +28,7 @@
 #include <rte_pdump.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
+#define CMD_LINE_OPT_MULTI "multi"
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -139,12 +140,14 @@ struct parse_val {
 static int num_tuples;
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
+static uint8_t multiple_core_capture;
 
 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options] -- [--multi] "
+			"--pdump "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -376,6 +379,7 @@ launch_args_parse(int argc, char **argv, char *prgname)
 	int option_index;
 	static struct option long_option[] = {
 		{"pdump", 1, 0, 0},
+		{"multi", 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -395,6 +399,10 @@ launch_args_parse(int argc, char **argv, char *prgname)
 					pdump_usage(prgname);
 					return -1;
 				}
+			} else if (!strncmp(long_option[option_index].name,
+					CMD_LINE_OPT_MULTI,
+					sizeof(CMD_LINE_OPT_MULTI))) {
+				multiple_core_capture = 1;
 			}
 			break;
 		default:
@@ -834,23 +842,69 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	printf(" core (%u); port %u device (%s) queue %u\n",
+			rte_lcore_id(), pt->port, pt->device_id, pt->queue);
+	fflush(stdout);
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	if (!multiple_core_capture) {
+		printf(" core (%u), capture for (%d) tuples\n",
+				rte_lcore_id(), num_tuples);
+		fflush(stdout);
 
-	while (!quit_signal) {
-		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++)
+				pdump_packets(&pdump_t[i]);
 		}
+
+		return;
 	}
+
+	/* check if there enough core */
+	if ((uint32_t)num_tuples >= rte_lcore_count()) {
+		printf("Insufficient cores to run parallel!\n");
+		return;
+	}
+
+	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+	for (i = 0; i < num_tuples; i++) {
+		rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			rte_exit(EXIT_FAILURE, "failed to wait\n");
+	}
+
+	/* master core */
+	while (!quit_signal)
+		;
 }
 
 int
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..dd60cb812 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [--multi]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,10 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``--multi`` command line option is optional argument. If passed, capture
+will be running on unqiue cores for all ``--pdump`` options. If ignored,
+capture will be running on single core for all ``--pdump`` options.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -112,4 +117,5 @@ Example
 
 .. code-block:: console
 
-   $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3 -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3,4,5 -- --multi --pdump 'port=0,queue=*,rx-dev=/tmp/rx-1.pcap' --pdump 'port=1,queue=*,rx-dev=/tmp/rx-2.pcap'
-- 
2.17.1

Re: [PATCH v4 2/2] app/pdump: enhance to support multi-core capture

[David Marchand]

On Tue, Apr 2, 2019 at 6:33 AM Vipin Varghese <vipin.varghese@intel.com>
wrote:

> Add option --multi, to enhance the pdump application to allow capture
> on unique cores for each --pdump option. If option --multi is ignored
> the default capture occurs on a single core for all --pdump options.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>  app/pdump/main.c           | 76 ++++++++++++++++++++++++++++++++------
>  doc/guides/tools/pdump.rst |  8 +++-
>  2 files changed, 72 insertions(+), 12 deletions(-)
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index c1db2eb8d..997c8942f 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -28,6 +28,7 @@
>  #include <rte_pdump.h>
>
>  #define CMD_LINE_OPT_PDUMP "pdump"
> +#define CMD_LINE_OPT_MULTI "multi"
>  #define PDUMP_PORT_ARG "port"
>  #define PDUMP_PCI_ARG "device_id"
>  #define PDUMP_QUEUE_ARG "queue"
> @@ -139,12 +140,14 @@ struct parse_val {
>  static int num_tuples;
>  static struct rte_eth_conf port_conf_default;
>  static volatile uint8_t quit_signal;
> +static uint8_t multiple_core_capture;
>
>  /**< display usage */
>  static void
>  pdump_usage(const char *prgname)
>  {
> -       printf("usage: %s [EAL options] -- --pdump "
> +       printf("usage: %s [EAL options] -- [--multi] "
> +                       "--pdump "
>                         "'(port=<port id> | device_id=<pci id or vdev
> name>),"
>                         "(queue=<queue_id>),"
>                         "(rx-dev=<iface or pcap file> |"
>

Rather than hardcode the usage, reuse the macro you introduced:
CMD_LINE_OPT_MULTI.


@@ -376,6 +379,7 @@ launch_args_parse(int argc, char **argv, char *prgname)
>         int option_index;
>         static struct option long_option[] = {
>                 {"pdump", 1, 0, 0},
> +               {"multi", 0, 0, 0},
>                 {NULL, 0, 0, 0}
>         };
>
>
Idem reuse the macro.

Besides look at lib/librte_eal/common/eal_options.h and
lib/librte_eal/common/eal_common_options.c.
Define an enum for long only options and map "multi" to an integer that has
no printable character associated.

This way, you can avoid (see below)...

@@ -395,6 +399,10 @@ launch_args_parse(int argc, char **argv, char *prgname)
>                                         pdump_usage(prgname);
>                                         return -1;
>                                 }
> +                       } else if (!strncmp(long_option[option_index].name,
> +                                       CMD_LINE_OPT_MULTI,
> +                                       sizeof(CMD_LINE_OPT_MULTI))) {
> +                               multiple_core_capture = 1;
>                         }
>                         break;
>                 default:
>

... this strncmp.
getopt_long already matched the input option for you.


-- 
David Marchand

Re: [PATCH v4 2/2] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi David,

Thanks for the inputs, these are useful. Let me work on these and share v5 ASAP.

Thanks
Vipin Varghese

From: David Marchand <david.marchand@redhat.com>
Sent: Tuesday, April 2, 2019 12:35 PM
To: Varghese, Vipin <vipin.varghese@intel.com>
Cc: dev <dev@dpdk.org>; Kovacevic, Marko <marko.kovacevic@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>; 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>
Subject: Re: [dpdk-dev] [PATCH v4 2/2] app/pdump: enhance to support multi-core capture


On Tue, Apr 2, 2019 at 6:33 AM Vipin Varghese <vipin.varghese@intel.com<mailto:vipin.varghese@intel.com>> wrote:
Add option --multi, to enhance the pdump application to allow capture
on unique cores for each --pdump option. If option --multi is ignored
the default capture occurs on a single core for all --pdump options.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com<mailto:vipin.varghese@intel.com>>
---
 app/pdump/main.c           | 76 ++++++++++++++++++++++++++++++++------
 doc/guides/tools/pdump.rst |  8 +++-
 2 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c1db2eb8d..997c8942f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -28,6 +28,7 @@
 #include <rte_pdump.h>

 #define CMD_LINE_OPT_PDUMP "pdump"
+#define CMD_LINE_OPT_MULTI "multi"
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -139,12 +140,14 @@ struct parse_val {
 static int num_tuples;
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
+static uint8_t multiple_core_capture;

 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
-       printf("usage: %s [EAL options] -- --pdump "
+       printf("usage: %s [EAL options] -- [--multi] "
+                       "--pdump "
                        "'(port=<port id> | device_id=<pci id or vdev name>),"
                        "(queue=<queue_id>),"
                        "(rx-dev=<iface or pcap file> |"

Rather than hardcode the usage, reuse the macro you introduced: CMD_LINE_OPT_MULTI.


@@ -376,6 +379,7 @@ launch_args_parse(int argc, char **argv, char *prgname)
        int option_index;
        static struct option long_option[] = {
                {"pdump", 1, 0, 0},
+               {"multi", 0, 0, 0},
                {NULL, 0, 0, 0}
        };

Idem reuse the macro.

Besides look at lib/librte_eal/common/eal_options.h and lib/librte_eal/common/eal_common_options.c.
Define an enum for long only options and map "multi" to an integer that has no printable character associated.

This way, you can avoid (see below)...

@@ -395,6 +399,10 @@ launch_args_parse(int argc, char **argv, char *prgname)
                                        pdump_usage(prgname);
                                        return -1;
                                }
+                       } else if (!strncmp(long_option[option_index].name,
+                                       CMD_LINE_OPT_MULTI,
+                                       sizeof(CMD_LINE_OPT_MULTI))) {
+                               multiple_core_capture = 1;
                        }
                        break;
                default:

... this strncmp.
getopt_long already matched the input option for you.


--
David Marchand

[PATCH v5 0/2] app/pdump: enhance to support unique cores

[Vipin Varghese]

The patch series enhances application to support pdump capture to run
on unique cores.

Motivation
==========

DPDK pdump capture tool currently runs as secondary on the default core 0.
For all --pdump, core 0 iterates and capture packets. This leads to drops
and delay in the capture. This introduces skew in result and performance
bottleneck.

In order to allow --pdump to unique cores, the pdump application has to be
support multi-core in EAL arguments and each --pdump has to be mapped to
unique cores.

Status
======

 - Default c_flag option is removed.
 - with option --multi, the application can run on unique cores.

Change Log:
===========

V5:
 - Re-use MACRO and replace strncmp - David Marchand

V4:
 - spelling correction - Reshma Pathan
 - remove c_falg - Reshma Pathan

V3:
 - correct the parse_usage - Vipin Varghese
 - add change log - Vipin Varghese

V2:
 - Replace option '-c' to '-l' - Keith Wiles

Vipin Varghese (2):
  app/pdump: remove core restriction
  app/pdump: enhance to support multi-core capture

 app/pdump/main.c           | 109 +++++++++++++++++++++++++++----------
 doc/guides/tools/pdump.rst |   8 ++-
 2 files changed, 86 insertions(+), 31 deletions(-)

Future:
=======
 - In multi mode, allow master to stop --pdump for desired file size.
 - Stop selective pdump.

-- 
2.17.1

[PATCH v5 1/2] app/pdump: remove core restriction

[Vipin Varghese]

PDUMP application is being limited to run on default first core.
The patch removes the restriction, allowing user to run on any of
selected cores in EAL args. If no args are passed, logic runs on
default master core.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..c1db2eb8d 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -860,23 +860,21 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
-	char *argp[argc + 3];
+	char *argp[argc + 2];
 
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
 	argp[0] = argv[0];
-	argp[1] = c_flag;
-	argp[2] = n_flag;
-	argp[3] = mp_flag;
+	argp[1] = n_flag;
+	argp[2] = mp_flag;
 
 	for (i = 1; i < argc; i++)
-		argp[i + 3] = argv[i];
+		argp[i + 2] = argv[i];
 
-	argc += 3;
+	argc += 2;
 
 	diag = rte_eal_init(argc, argp);
 	if (diag < 0)
@@ -886,7 +884,7 @@ main(int argc, char **argv)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
 
 	argc -= diag;
-	argv += (diag - 3);
+	argv += (diag - 2);
 
 	/* parse app arguments */
 	if (argc > 1) {
-- 
2.17.1

[PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Add option --multi, to enhance pdump application to allow capture
on unique cores for each --pdump option. If option --multi is ignored
the default capture occurs on single core for all --pdump options.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c           | 95 +++++++++++++++++++++++++++++---------
 doc/guides/tools/pdump.rst |  8 +++-
 2 files changed, 80 insertions(+), 23 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c1db2eb8d..0d12cee3f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -28,6 +28,9 @@
 #include <rte_pdump.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
+#define CMD_LINE_OPT_PDUMP_NUM 1
+#define CMD_LINE_OPT_MULTI "multi"
+#define CMD_LINE_OPT_MULTI_NUM 2
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -139,12 +142,14 @@ struct parse_val {
 static int num_tuples;
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
+static uint8_t multiple_core_capture;
 
 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options] -- [--%s] "
+			"--%s "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
 			"[ring-size=<ring size>default:16384],"
 			"[mbuf-size=<mbuf data size>default:2176],"
 			"[total-num-mbufs=<number of mbufs>default:65535]'\n",
-			prgname);
+			prgname, CMD_LINE_OPT_MULTI, CMD_LINE_OPT_PDUMP);
 }
 
 static int
@@ -375,7 +380,8 @@ launch_args_parse(int argc, char **argv, char *prgname)
 	int opt, ret;
 	int option_index;
 	static struct option long_option[] = {
-		{"pdump", 1, 0, 0},
+		{CMD_LINE_OPT_PDUMP, 1, 0, CMD_LINE_OPT_PDUMP_NUM},
+		{CMD_LINE_OPT_MULTI, 0, 0, CMD_LINE_OPT_MULTI_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -386,17 +392,16 @@ launch_args_parse(int argc, char **argv, char *prgname)
 	while ((opt = getopt_long(argc, argv, " ",
 			long_option, &option_index)) != EOF) {
 		switch (opt) {
-		case 0:
-			if (!strncmp(long_option[option_index].name,
-					CMD_LINE_OPT_PDUMP,
-					sizeof(CMD_LINE_OPT_PDUMP))) {
-				ret = parse_pdump(optarg);
-				if (ret) {
-					pdump_usage(prgname);
-					return -1;
-				}
+		case CMD_LINE_OPT_PDUMP_NUM:
+			ret = parse_pdump(optarg);
+			if (ret) {
+				pdump_usage(prgname);
+				return -1;
 			}
 			break;
+		case CMD_LINE_OPT_MULTI_NUM:
+			multiple_core_capture = 1;
+			break;
 		default:
 			pdump_usage(prgname);
 			return -1;
@@ -834,23 +839,69 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	printf(" core (%u); port %u device (%s) queue %u\n",
+			rte_lcore_id(), pt->port, pt->device_id, pt->queue);
+	fflush(stdout);
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	if (!multiple_core_capture) {
+		printf(" core (%u), capture for (%d) tuples\n",
+				rte_lcore_id(), num_tuples);
+		fflush(stdout);
 
-	while (!quit_signal) {
-		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++)
+				pdump_packets(&pdump_t[i]);
 		}
+
+		return;
+	}
+
+	/* check if there enough core */
+	if ((uint32_t)num_tuples >= rte_lcore_count()) {
+		printf("Insufficient cores to run parallel!\n");
+		return;
 	}
+
+	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+	for (i = 0; i < num_tuples; i++) {
+		rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			rte_exit(EXIT_FAILURE, "failed to wait\n");
+	}
+
+	/* master core */
+	while (!quit_signal)
+		;
 }
 
 int
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..dd60cb812 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [--multi]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,10 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``--multi`` command line option is optional argument. If passed, capture
+will be running on unqiue cores for all ``--pdump`` options. If ignored,
+capture will be running on single core for all ``--pdump`` options.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -112,4 +117,5 @@ Example
 
 .. code-block:: console
 
-   $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3 -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3,4,5 -- --multi --pdump 'port=0,queue=*,rx-dev=/tmp/rx-1.pcap' --pdump 'port=1,queue=*,rx-dev=/tmp/rx-2.pcap'
-- 
2.17.1

Re: [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[David Marchand]

On Tue, Apr 2, 2019 at 11:18 AM Vipin Varghese <vipin.varghese@intel.com>
wrote:

> @@ -28,6 +28,9 @@
>  #include <rte_pdump.h>
>
>  #define CMD_LINE_OPT_PDUMP "pdump"
> +#define CMD_LINE_OPT_PDUMP_NUM 1
> +#define CMD_LINE_OPT_MULTI "multi"
> +#define CMD_LINE_OPT_MULTI_NUM 2
>  #define PDUMP_PORT_ARG "port"
>  #define PDUMP_PCI_ARG "device_id"
>  #define PDUMP_QUEUE_ARG "queue"
>

You'd better map to integers that do not collide with ascii characters
(even if non printable).
So values >= 256 are better.
This is the reason for the comment here:
https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h#n19


@@ -139,12 +142,14 @@ struct parse_val {
>  static int num_tuples;
>  static struct rte_eth_conf port_conf_default;
>  static volatile uint8_t quit_signal;
> +static uint8_t multiple_core_capture;
>
>  /**< display usage */
>  static void
>  pdump_usage(const char *prgname)
>  {
> -       printf("usage: %s [EAL options] -- --pdump "
> +       printf("usage: %s [EAL options] -- [--%s] "
> +                       "--%s "
>                         "'(port=<port id> | device_id=<pci id or vdev
> name>),"
>                         "(queue=<queue_id>),"
>                         "(rx-dev=<iface or pcap file> |"
> @@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
>                         "[ring-size=<ring size>default:16384],"
>                         "[mbuf-size=<mbuf data size>default:2176],"
>                         "[total-num-mbufs=<number of
> mbufs>default:65535]'\n",
> -                       prgname);
> +                       prgname, CMD_LINE_OPT_MULTI, CMD_LINE_OPT_PDUMP);
>  }
>
>  static int
>

You can concatenate the macro.

-       printf("usage: %s [EAL options] -- --pdump "
+       printf("usage: %s [EAL options] -- [--"CMD_LINE_OPT_MULTI"] "
+                       "--"CMD_LINE_OPT_PDUMP" "



-- 
David Marchand

Re: [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi David,

snipped
 #define CMD_LINE_OPT_PDUMP "pdump"
+#define CMD_LINE_OPT_PDUMP_NUM 1
+#define CMD_LINE_OPT_MULTI "multi"
+#define CMD_LINE_OPT_MULTI_NUM 2
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"

You'd better map to integers that do not collide with ascii characters (even if non printable).
So values >= 256 are better.
This is the reason for the comment here:
https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h#n19

In case of dpdk-pdump; there are 2 options ‘multi’ and ‘pdump’. But in case of eal_option there are multiple options which has same fist character. Hence the comment 'first long only option value must be >= 256, so that we won't conflict with short options' is true.

In my humble opinion, I think it need not change. But please let me know otherwise.
Snipped
+       printf("usage: %s [EAL options] -- [--%s] "
+                       "--%s "
                        "'(port=<port id> | device_id=<pci id or vdev name>),"
                        "(queue=<queue_id>),"
                        "(rx-dev=<iface or pcap file> |"
@@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
                        "[ring-size=<ring size>default:16384],"
                        "[mbuf-size=<mbuf data size>default:2176],"
                        "[total-num-mbufs=<number of mbufs>default:65535]'\n",
-                       prgname);
+                       prgname, CMD_LINE_OPT_MULTI, CMD_LINE_OPT_PDUMP);
 }

 static int

You can concatenate the macro.

Thank you for the suggestion,

#define OPT(X) #X
#define STR_REPLACE "\n[--"OPT(multi)"]\n --"OPT(pdump)

prgname, STR_REPLACE);

should we change to this or skip this as we are using this once?

snipped

Re: [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[David Marchand]

On Tue, Apr 2, 2019 at 5:30 PM Varghese, Vipin <vipin.varghese@intel.com>
wrote:

> Hi David,
>
>
>
> snipped
>  #define CMD_LINE_OPT_PDUMP "pdump"
> +#define CMD_LINE_OPT_PDUMP_NUM 1
> +#define CMD_LINE_OPT_MULTI "multi"
> +#define CMD_LINE_OPT_MULTI_NUM 2
>  #define PDUMP_PORT_ARG "port"
>  #define PDUMP_PCI_ARG "device_id"
>  #define PDUMP_QUEUE_ARG "queue"
>
>
>
> You'd better map to integers that do not collide with ascii characters
> (even if non printable).
>
> So values >= 256 are better.
>
> This is the reason for the comment here:
>
> https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h#n19
>
>
>
> In case of dpdk-pdump; there are 2 options ‘multi’ and ‘pdump’. But in
> case of eal_option there are multiple options which has same fist
> character. Hence the comment 'first long only option value must be >= 256,
> so that we won't conflict with short options' is true.
>
>
>
> In my humble opinion, I think it need not change. But please let me know
> otherwise.
>

The convention I had proposed is to just leave the whole [0-255] range to
potential short options.
This marks a demarcation between long options that map to short options and
long "only" options.
I don't care, just prefer to have something systematic.


Snipped
>
> +       printf("usage: %s [EAL options] -- [--%s] "
> +                       "--%s "
>                         "'(port=<port id> | device_id=<pci id or vdev
> name>),"
>                         "(queue=<queue_id>),"
>                         "(rx-dev=<iface or pcap file> |"
> @@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
>                         "[ring-size=<ring size>default:16384],"
>                         "[mbuf-size=<mbuf data size>default:2176],"
>                         "[total-num-mbufs=<number of
> mbufs>default:65535]'\n",
> -                       prgname);
> +                       prgname, CMD_LINE_OPT_MULTI, CMD_LINE_OPT_PDUMP);
>  }
>
>  static int
>
>
>
> You can concatenate the macro.
>
>
>
> Thank you for the suggestion,
>
>
>
> #define OPT(X) #X
>
> #define STR_REPLACE "\n[--"OPT(multi)"]\n --"OPT(pdump)
>
>
> prgname, STR_REPLACE);
>
>
>
> should we change to this or skip this as we are using this once?
>
>
>
> snipped
>

I don't understand what you propose.
My suggestion is simple:
+       printf("usage: %s [EAL options] -- [--"CMD_LINE_OPT_MULTI"] "
+                       "--"CMD_LINE_OPT_PDUMP" "


-- 
David Marchand

Re: [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> 
> @@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
>  			"[ring-size=<ring size>default:16384],"
>  			"[mbuf-size=<mbuf data size>default:2176],"
>  			"[total-num-mbufs=<number of
> mbufs>default:65535]'\n",
> -			prgname);
> +			prgname, CMD_LINE_OPT_MULTI,

IMO, simple  short options can be used instead of long, as this option don't have any arguments to pass.

> 
> +static int
> +dump_packets_core(void *arg)
> +{
> +	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
> +
> +	printf(" core (%u); port %u device (%s) queue %u\n",
> +			rte_lcore_id(), pt->port, pt->device_id, pt->queue);

Log message can be improved to be "  packet_dump for port <num> running on core <id>"

> +	fflush(stdout);

Why is fflush used here and  in below other places?

> 
> +The ``--multi`` command line option is optional argument. If passed,
> +capture will be running on unqiue cores for all ``--pdump`` options. If

Typo unique

Thanks,
Reshma

Re: [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi Reshma,

snipped
> > +
> > +	printf(" core (%u); port %u device (%s) queue %u\n",
> > +			rte_lcore_id(), pt->port, pt->device_id, pt->queue);
> 
> Log message can be improved to be "  packet_dump for port <num> running on
> core <id>"
Thanks for the suggestion, but I am comfortable with this message.

> 
> > +	fflush(stdout);
> 
> Why is fflush used here and  in below other places?
To ensure the stdout content is flushed out. We had used similar approach to ' examples/l2fwd/main.c'

> 
> >
> > +The ``--multi`` command line option is optional argument. If passed,
> > +capture will be running on unqiue cores for all ``--pdump`` options.
> > +If
> 
> Typo unique
> 
> Thanks,
> Reshma

Re: [dpdk-dev] [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> >
> > > +	fflush(stdout);
> >
> > Why is fflush used here and  in below other places?
> To ensure the stdout content is flushed out. We had used similar approach to '
> examples/l2fwd/main.c'
> 

Can you elaborate more?  What problem do you see if you don't use this?

Thanks,
Reshma 

Re: [dpdk-dev] [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi Reshma,

snipped
> > >
> > > > +	fflush(stdout);
> > >
> > > Why is fflush used here and  in below other places?
> > To ensure the stdout content is flushed out. We had used similar approach to '
> > examples/l2fwd/main.c'
> >
> 
> Can you elaborate more?  What problem do you see if you don't use this?
Sure, when running on multi cores (Xeon, corei7 and denverton alike) depending upon the isolate parameters and other application, printf from lcores either comes as partial or misaligned. Making use 'fflush' ensured in both apps and examples to be flushed out. 

> 
> Thanks,
> Reshma

[PATCH v6 0/2] app/pdump: enhance to support unique cores

[Vipin Varghese]

The patch series enhances application to support pdump capture to run
on unique cores.

Motivation
==========

DPDK pdump capture tool currently runs as secondary on the default core 0.
For all --pdump, core 0 iterates and capture packets. This leads to drops
and delay in the capture. This introduces skew in result and performance
bottleneck.

In order to allow --pdump to unique cores, the pdump application has to be
support multi-core in EAL arguments and each --pdump has to be mapped to
unique cores.

Status
======

 - Default c_flag option is removed.
 - with option --multi, the application can run on unique cores.

Change Log:
===========

V6:
 - change MACRO value - David Marchand
 - use MACRO in usage function - David Marchand
 - correct spelling in documentation - Reshma Pathan

V5:
 - Re-use MACRO and replace strncmp - David Marchand

V4:
 - spelling correction - Reshma Pathan
 - remove c_falg - Reshma Pathan

V3:
 - correct the parse_usage - Vipin Varghese
 - add change log - Vipin Varghese

V2:
 - Replace option '-c' to '-l' - Keith Wiles

Vipin Varghese (2):
  app/pdump: remove core restriction
  app/pdump: enhance to support multi-core capture

 app/pdump/main.c           | 113 +++++++++++++++++++++++++++----------
 doc/guides/tools/pdump.rst |   8 ++-
 2 files changed, 91 insertions(+), 30 deletions(-)

Future:
=======
 - Ehance multi mode, allow master to stop --pdump for desired file size.
 - Ehance multi mode, allow to suggest NUMA aware file locations.
 - Stop unique pdump.

-- 
2.17.1

[PATCH v6 1/2] app/pdump: remove core restriction

[Vipin Varghese]

PDUMP application is being limited to run on default first core.
The patch removes the restriction, allowing user to run on any of
selected cores in EAL args. If no args are passed, logic runs on
default master core.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index ccf2a1d2f..c1db2eb8d 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -860,23 +860,21 @@ main(int argc, char **argv)
 	int ret;
 	int i;
 
-	char c_flag[] = "-c1";
 	char n_flag[] = "-n4";
 	char mp_flag[] = "--proc-type=secondary";
-	char *argp[argc + 3];
+	char *argp[argc + 2];
 
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, signal_handler);
 
 	argp[0] = argv[0];
-	argp[1] = c_flag;
-	argp[2] = n_flag;
-	argp[3] = mp_flag;
+	argp[1] = n_flag;
+	argp[2] = mp_flag;
 
 	for (i = 1; i < argc; i++)
-		argp[i + 3] = argv[i];
+		argp[i + 2] = argv[i];
 
-	argc += 3;
+	argc += 2;
 
 	diag = rte_eal_init(argc, argp);
 	if (diag < 0)
@@ -886,7 +884,7 @@ main(int argc, char **argv)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
 
 	argc -= diag;
-	argv += (diag - 3);
+	argv += (diag - 2);
 
 	/* parse app arguments */
 	if (argc > 1) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v6 1/2] app/pdump: remove core restriction

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, April 4, 2019 9:55 AM
> To: dev@dpdk.org; Kovacevic, Marko <marko.kovacevic@intel.com>; Pattan,
> Reshma <reshma.pattan@intel.com>; david.marchand@redhat.com
> 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>; Varghese,
> Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v6 1/2] app/pdump: remove core restriction
> 
> PDUMP application is being limited to run on default first core.
> The patch removes the restriction, allowing user to run on any of selected cores
> in EAL args. If no args are passed, logic runs on default master core.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>

[PATCH v6 2/2] app/pdump: enhance to support multi-core capture

[Vipin Varghese]

Add option --multi, to enhance pdump application to allow capture
on unique cores for each --pdump option. If option --multi is ignored
the default capture occurs on single core for all --pdump options.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/pdump/main.c           | 99 ++++++++++++++++++++++++++++++--------
 doc/guides/tools/pdump.rst |  8 ++-
 2 files changed, 85 insertions(+), 22 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c1db2eb8d..ae2c01b7b 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -28,6 +28,9 @@
 #include <rte_pdump.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
+#define CMD_LINE_OPT_PDUMP_NUM 256
+#define CMD_LINE_OPT_MULTI "multi"
+#define CMD_LINE_OPT_MULTI_NUM 257
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -139,12 +142,15 @@ struct parse_val {
 static int num_tuples;
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
+static uint8_t multiple_core_capture;
 
 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
-	printf("usage: %s [EAL options] -- --pdump "
+	printf("usage: %s [EAL options]"
+			" --["CMD_LINE_OPT_MULTI"]\n"
+			" --"CMD_LINE_OPT_PDUMP" "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
 			"(queue=<queue_id>),"
 			"(rx-dev=<iface or pcap file> |"
@@ -375,7 +381,8 @@ launch_args_parse(int argc, char **argv, char *prgname)
 	int opt, ret;
 	int option_index;
 	static struct option long_option[] = {
-		{"pdump", 1, 0, 0},
+		{CMD_LINE_OPT_PDUMP, 1, 0, CMD_LINE_OPT_PDUMP_NUM},
+		{CMD_LINE_OPT_MULTI, 0, 0, CMD_LINE_OPT_MULTI_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -386,17 +393,16 @@ launch_args_parse(int argc, char **argv, char *prgname)
 	while ((opt = getopt_long(argc, argv, " ",
 			long_option, &option_index)) != EOF) {
 		switch (opt) {
-		case 0:
-			if (!strncmp(long_option[option_index].name,
-					CMD_LINE_OPT_PDUMP,
-					sizeof(CMD_LINE_OPT_PDUMP))) {
-				ret = parse_pdump(optarg);
-				if (ret) {
-					pdump_usage(prgname);
-					return -1;
-				}
+		case CMD_LINE_OPT_PDUMP_NUM:
+			ret = parse_pdump(optarg);
+			if (ret) {
+				pdump_usage(prgname);
+				return -1;
 			}
 			break;
+		case CMD_LINE_OPT_MULTI_NUM:
+			multiple_core_capture = 1;
+			break;
 		default:
 			pdump_usage(prgname);
 			return -1;
@@ -834,23 +840,74 @@ enable_pdump(void)
 	}
 }
 
+static inline void
+pdump_packets(struct pdump_tuples *pt)
+{
+	if (pt->dir & RTE_PDUMP_FLAG_RX)
+		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
+	if (pt->dir & RTE_PDUMP_FLAG_TX)
+		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+}
+
+static int
+dump_packets_core(void *arg)
+{
+	struct pdump_tuples *pt = (struct pdump_tuples *) arg;
+
+	printf(" core (%u); port %u device (%s) queue %u\n",
+			rte_lcore_id(), pt->port, pt->device_id, pt->queue);
+	fflush(stdout);
+
+	while (!quit_signal)
+		pdump_packets(pt);
+
+	return 0;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	struct pdump_tuples *pt;
+	uint32_t lcore_id = 0;
+
+	if (!multiple_core_capture) {
+		printf(" core (%u), capture for (%d) tuples\n",
+				rte_lcore_id(), num_tuples);
+
+		for (i = 0; i < num_tuples; i++)
+			printf(" - port %u device (%s) queue %u\n",
+				pdump_t[i].port,
+				pdump_t[i].device_id,
+				pdump_t[i].queue);
 
-	while (!quit_signal) {
-		for (i = 0; i < num_tuples; i++) {
-			pt = &pdump_t[i];
-			if (pt->dir & RTE_PDUMP_FLAG_RX)
-				pdump_rxtx(pt->rx_ring, pt->rx_vdev_id,
-					&pt->stats);
-			if (pt->dir & RTE_PDUMP_FLAG_TX)
-				pdump_rxtx(pt->tx_ring, pt->tx_vdev_id,
-					&pt->stats);
+		while (!quit_signal) {
+			for (i = 0; i < num_tuples; i++)
+				pdump_packets(&pdump_t[i]);
 		}
+
+		return;
+	}
+
+	/* check if there enough core */
+	if ((uint32_t)num_tuples >= rte_lcore_count()) {
+		printf("Insufficient cores to run parallel!\n");
+		return;
 	}
+
+	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+	for (i = 0; i < num_tuples; i++) {
+		rte_eal_remote_launch(dump_packets_core,
+				&pdump_t[i], lcore_id);
+		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			rte_exit(EXIT_FAILURE, "failed to wait\n");
+	}
+
+	/* master core */
+	while (!quit_signal)
+		;
 }
 
 int
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 7c2b73e72..53cd2b464 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -35,6 +35,7 @@ The tool has a number of command line options:
 .. code-block:: console
 
    ./build/app/dpdk-pdump --
+                          [--multi]
                           --pdump '(port=<port id> | device_id=<pci id or vdev name>),
                                    (queue=<queue_id>),
                                    (rx-dev=<iface or pcap file> |
@@ -43,6 +44,10 @@ The tool has a number of command line options:
                                    [mbuf-size=<mbuf data size>],
                                    [total-num-mbufs=<number of mbufs>]'
 
+The ``--multi`` command line option is optional argument. If passed, capture
+will be running on unique cores for all ``--pdump`` options. If ignored,
+capture will be running on single core for all ``--pdump`` options.
+
 The ``--pdump`` command line option is mandatory and it takes various sub arguments which are described in
 below section.
 
@@ -112,4 +117,5 @@ Example
 
 .. code-block:: console
 
-   $ sudo ./build/app/dpdk-pdump -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3 -- --pdump 'port=0,queue=*,rx-dev=/tmp/rx.pcap'
+   $ sudo ./build/app/dpdk-pdump -l 3,4,5 -- --multi --pdump 'port=0,queue=*,rx-dev=/tmp/rx-1.pcap' --pdump 'port=1,queue=*,rx-dev=/tmp/rx-2.pcap'
-- 
2.17.1

Re: [dpdk-dev] [PATCH v6 2/2] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> 


Nit picks:

> 
> +The ``--multi`` command line option is optional argument. If passed,
> +capture will be running on unique cores for all ``--pdump`` options. If
> +ignored, capture will be running on single core for all ``--pdump`` options.
> +

Instead of just saying on single cores or on unique cores, worth mentioning  uses the 1st core of eal coremask/corelist for single case.
For multi case uses the cores except the 1st in one in coremask? 

>  The ``--pdump`` command line option is mandatory and it takes various sub
> arguments which are described in  below section.


> +   $ sudo ./build/app/dpdk-pdump -l 3,4,5 -- --multi --pdump
> 'port=0,queue=*,rx-dev=/tmp/rx-1.pcap' --pdump 'port=1,queue=*,rx-
> dev=/tmp/rx-2.pcap'
> --
> 2.17.1

In the command if you pass anything --m/--mul/--mult/multi the command still succeeds..  Did you check the reason for that?

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v6 2/2] app/pdump: enhance to support multi-core capture

[Varghese, Vipin]

Hi Reshma,

snipped
> Nit picks:
> 
> >
> > +The ``--multi`` command line option is optional argument. If passed,
> > +capture will be running on unique cores for all ``--pdump`` options.
> > +If ignored, capture will be running on single core for all ``--pdump`` options.
> > +
> 
> Instead of just saying on single cores or on unique cores, worth mentioning  uses
> the 1st core of eal coremask/corelist for single case.
I have humbly disagree with this point, since the value of 1st core varies with isol, taskset, and eal option '--master'. 

> For multi case uses the cores except the 1st in one in coremask?
Same as above.

> 
> >  The ``--pdump`` command line option is mandatory and it takes various
> > sub arguments which are described in  below section.
> 
> 
> > +   $ sudo ./build/app/dpdk-pdump -l 3,4,5 -- --multi --pdump
> > 'port=0,queue=*,rx-dev=/tmp/rx-1.pcap' --pdump 'port=1,queue=*,rx-
> > dev=/tmp/rx-2.pcap'
> > --
> > 2.17.1
> 
> In the command if you pass anything --m/--mul/--mult/multi the command still
> succeeds..  Did you check the reason for that?
Yes it passes, during unit test checked too. But I do not understand if it is problem.

> 
> Thanks,
> Reshma

Re: [dpdk-dev] [PATCH v6 2/2] app/pdump: enhance to support multi-core capture

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, April 4, 2019 9:55 AM
> To: dev@dpdk.org; Kovacevic, Marko <marko.kovacevic@intel.com>; Pattan,
> Reshma <reshma.pattan@intel.com>; david.marchand@redhat.com
> 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>; Varghese,
> Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v6 2/2] app/pdump: enhance to support multi-core capture
> 
> Add option --multi, to enhance pdump application to allow capture on unique
> cores for each --pdump option. If option --multi is ignored the default capture
> occurs on single core for all --pdump options.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH v6 0/2] app/pdump: enhance to support unique cores

[Thomas Monjalon]

04/04/2019 10:55, Vipin Varghese:
> The patch series enhances application to support pdump capture to run
> on unique cores.
> 
> Motivation
> ==========
> 
> DPDK pdump capture tool currently runs as secondary on the default core 0.
> For all --pdump, core 0 iterates and capture packets. This leads to drops
> and delay in the capture. This introduces skew in result and performance
> bottleneck.
> 
> In order to allow --pdump to unique cores, the pdump application has to be
> support multi-core in EAL arguments and each --pdump has to be mapped to
> unique cores.

Applied, thanks