[v5,2/2] app/pdump: enhance to support multi-core capture
Checks
Commit Message
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(-)
Comments
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" "
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
> -----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
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
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" "
> -----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
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
@@ -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
@@ -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'