[v5,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>
---
 app/pdump/main.c           | 95 +++++++++++++++++++++++++++++---------
 doc/guides/tools/pdump.rst |  8 +++-
 2 files changed, 80 insertions(+), 23 deletions(-)
  

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