DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Varghese, Vipin" <vipin.varghese@intel.com>,
	"Pattan, Reshma" <reshma.pattan@intel.com>,
	"dev@dpdk.org" <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
Date: Fri, 29 Mar 2019 17:03:03 +0000	[thread overview]
Message-ID: <424e5133-7918-7efe-330b-b4393f84a563@intel.com> (raw)
Message-ID: <20190329170303.LGB1pPEi4136D-YnlEiZiVSr8N-0CkkqWIGGLzIWQgE@z> (raw)
In-Reply-To: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D32780A@BGSMSX101.gar.corp.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
> 


  parent reply	other threads:[~2019-03-29 17:03 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-28  6:00 [dpdk-dev] [PATCH] " Vipin Varghese
2019-03-28  6:00 ` Vipin Varghese
2019-03-28 14:57 ` [dpdk-dev] [PATCH v2] " Vipin Varghese
2019-03-28 14:57   ` Vipin Varghese
2019-03-28 15:04   ` [dpdk-dev] [PATCH v3] " Vipin Varghese
2019-03-28 15:04     ` Vipin Varghese
2019-03-28 15:34     ` Wiles, Keith
2019-03-28 15:34       ` Wiles, Keith
2019-03-29 10:08     ` Pattan, Reshma
2019-03-29 10:08       ` Pattan, Reshma
2019-03-29 10:22       ` Varghese, Vipin
2019-03-29 10:22         ` Varghese, Vipin
2019-03-29 10:52         ` Pattan, Reshma
2019-03-29 10:52           ` Pattan, Reshma
2019-03-29 17:03         ` Ferruh Yigit [this message]
2019-03-29 17:03           ` Ferruh Yigit
2019-04-01  4:05           ` Varghese, Vipin
2019-04-01  4:05             ` Varghese, Vipin
2019-04-02  4:33     ` [dpdk-dev] [PATCH v4 0/2] app/pdump: enhance to support unique cores Vipin Varghese
2019-04-02  4:33       ` Vipin Varghese
2019-04-02  4:33       ` [dpdk-dev] [PATCH v4 1/2] app/pdump: remove core restriction Vipin Varghese
2019-04-02  4:33         ` Vipin Varghese
2019-04-02  4:33       ` [dpdk-dev] [PATCH v4 2/2] app/pdump: enhance to support multi-core capture Vipin Varghese
2019-04-02  4:33         ` Vipin Varghese
2019-04-02  7:05         ` David Marchand
2019-04-02  7:05           ` David Marchand
2019-04-02  8:06           ` Varghese, Vipin
2019-04-02  8:06             ` Varghese, Vipin
2019-04-02  9:18         ` [dpdk-dev] [PATCH v5 0/2] app/pdump: enhance to support unique cores Vipin Varghese
2019-04-02  9:18           ` Vipin Varghese
2019-04-02  9:18           ` [dpdk-dev] [PATCH v5 1/2] app/pdump: remove core restriction Vipin Varghese
2019-04-02  9:18             ` Vipin Varghese
2019-04-02  9:18           ` [dpdk-dev] [PATCH v5 2/2] app/pdump: enhance to support multi-core capture Vipin Varghese
2019-04-02  9:18             ` Vipin Varghese
2019-04-02 10:01             ` David Marchand
2019-04-02 10:01               ` David Marchand
2019-04-02 15:30               ` Varghese, Vipin
2019-04-02 15:30                 ` Varghese, Vipin
2019-04-04  7:39                 ` David Marchand
2019-04-04  7:39                   ` David Marchand
2019-04-02 16:13             ` Pattan, Reshma
2019-04-02 16:13               ` Pattan, Reshma
2019-04-03  3:53               ` Varghese, Vipin
2019-04-03  3:53                 ` Varghese, Vipin
2019-04-05 17:10                 ` Pattan, Reshma
2019-04-05 17:10                   ` Pattan, Reshma
2019-04-08  3:03                   ` Varghese, Vipin
2019-04-08  3:03                     ` Varghese, Vipin
2019-04-04  8:55             ` [dpdk-dev] [PATCH v6 0/2] app/pdump: enhance to support unique cores Vipin Varghese
2019-04-04  8:55               ` Vipin Varghese
2019-04-04  8:55               ` [dpdk-dev] [PATCH v6 1/2] app/pdump: remove core restriction Vipin Varghese
2019-04-04  8:55                 ` Vipin Varghese
2019-04-09  9:04                 ` Pattan, Reshma
2019-04-09  9:04                   ` Pattan, Reshma
2019-04-04  8:55               ` [dpdk-dev] [PATCH v6 2/2] app/pdump: enhance to support multi-core capture Vipin Varghese
2019-04-04  8:55                 ` Vipin Varghese
2019-04-05 17:09                 ` Pattan, Reshma
2019-04-05 17:09                   ` Pattan, Reshma
2019-04-08  3:01                   ` Varghese, Vipin
2019-04-08  3:01                     ` Varghese, Vipin
2019-04-09  9:05                 ` Pattan, Reshma
2019-04-09  9:05                   ` Pattan, Reshma
2019-04-22 19:49               ` [dpdk-dev] [PATCH v6 0/2] app/pdump: enhance to support unique cores Thomas Monjalon
2019-04-22 19:49                 ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=424e5133-7918-7efe-330b-b4393f84a563@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=amit.tamboli@intel.com \
    --cc=amol.patel@intel.com \
    --cc=dev@dpdk.org \
    --cc=john.mcnamara@intel.com \
    --cc=keith.wiles@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=reshma.pattan@intel.com \
    --cc=sanjay.padubidri@intel.com \
    --cc=stephen1.byrne@intel.com \
    --cc=vipin.varghese@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).