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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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
>