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

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----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