diff mbox series

[v4,3/3] app/testpmd: set packet dump based on verbosity level

Message ID 1538897848-1693-3-git-send-email-rasland@mellanox.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v4,1/3] app/testpmd: move dumping packets to a separate function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Raslan Darawsheh Oct. 7, 2018, 7:38 a.m. UTC 
  when changing verbosity level it will configure rx/tx callbacks to dump
packets based on the verbosity value as following:
    1- dump only received packets:
       testpmd> set verbose 1
    2- dump only sent packets:
       testpmd> set verbose 2
    3- dump sent and received packets:
       testpmd> set verbose (any number > 2)
    4- disable dump
       testpmd> set verbose 0

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 app/test-pmd/config.c  | 25 +++++++++++++++++++++++++
 app/test-pmd/testpmd.c |  4 ++--
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)

Comments

Iremonger, Bernard Oct. 8, 2018, 10:04 a.m. UTC | #1 
> -----Original Message-----
> From: Raslan Darawsheh [mailto:rasland@mellanox.com]
> Sent: Sunday, October 7, 2018 8:38 AM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Shahaf Shuler
> <shahafs@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Xueming(Steven) Li <xuemingl@mellanox.com>; Ori Kam
> <orika@mellanox.com>; jerin.jacob@caviumnetworks.com;
> david.marchand@6wind.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Subject: [PATCH v4 3/3] app/testpmd: set packet dump based on verbosity level
> 
> when changing verbosity level it will configure rx/tx callbacks to dump packets
> based on the verbosity value as following:
>     1- dump only received packets:
>        testpmd> set verbose 1
>     2- dump only sent packets:
>        testpmd> set verbose 2
>     3- dump sent and received packets:
>        testpmd> set verbose (any number > 2)
>     4- disable dump
>        testpmd> set verbose 0
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
Ferruh Yigit Oct. 11, 2018, 3 p.m. UTC | #2 
On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
> when changing verbosity level it will configure rx/tx callbacks to dump
> packets based on the verbosity value as following:
>     1- dump only received packets:
>        testpmd> set verbose 1
>     2- dump only sent packets:
>        testpmd> set verbose 2
>     3- dump sent and received packets:
>        testpmd> set verbose (any number > 2)
>     4- disable dump
>        testpmd> set verbose 0

It is good to able to enable Rx/Tx separately but you are overloading "verbose"
meaning here. "verbose" is kind of log_level internal to testpmd and can be used
by many testpmd debug log.

Why not create a separate setting for it, perhaps like pkt_verbose?
Iremonger, Bernard Oct. 11, 2018, 3:24 p.m. UTC | #3 
Hi Feruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, October 11, 2018 4:00 PM
> To: Raslan Darawsheh <rasland@mellanox.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Shahaf Shuler
> <shahafs@mellanox.com>; Xueming(Steven) Li <xuemingl@mellanox.com>; Ori
> Kam <orika@mellanox.com>; jerin.jacob@caviumnetworks.com;
> david.marchand@6wind.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 3/3] app/testpmd: set packet dump based on
> verbosity level
> 
> On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
> > when changing verbosity level it will configure rx/tx callbacks to
> > dump packets based on the verbosity value as following:
> >     1- dump only received packets:
> >        testpmd> set verbose 1
> >     2- dump only sent packets:
> >        testpmd> set verbose 2
> >     3- dump sent and received packets:
> >        testpmd> set verbose (any number > 2)
> >     4- disable dump
> >        testpmd> set verbose 0
> 
> It is good to able to enable Rx/Tx separately but you are overloading "verbose"
> meaning here. "verbose" is kind of log_level internal to testpmd and can be used
> by many testpmd debug log.
> 
> Why not create a separate setting for it, perhaps like pkt_verbose?

The original code in rxonly.c used the verbose level, I don't think it is good idea to add a new pkt_verbose.
Users will be expecting it to work as before.

Regards,

Bernard.
Ferruh Yigit Oct. 11, 2018, 3:52 p.m. UTC | #4 
On 10/11/2018 4:24 PM, Iremonger, Bernard wrote:
> Hi Feruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Thursday, October 11, 2018 4:00 PM
>> To: Raslan Darawsheh <rasland@mellanox.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Shahaf Shuler
>> <shahafs@mellanox.com>; Xueming(Steven) Li <xuemingl@mellanox.com>; Ori
>> Kam <orika@mellanox.com>; jerin.jacob@caviumnetworks.com;
>> david.marchand@6wind.com; Iremonger, Bernard
>> <bernard.iremonger@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v4 3/3] app/testpmd: set packet dump based on
>> verbosity level
>>
>> On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
>>> when changing verbosity level it will configure rx/tx callbacks to
>>> dump packets based on the verbosity value as following:
>>>     1- dump only received packets:
>>>        testpmd> set verbose 1
>>>     2- dump only sent packets:
>>>        testpmd> set verbose 2
>>>     3- dump sent and received packets:
>>>        testpmd> set verbose (any number > 2)
>>>     4- disable dump
>>>        testpmd> set verbose 0
>>
>> It is good to able to enable Rx/Tx separately but you are overloading "verbose"
>> meaning here. "verbose" is kind of log_level internal to testpmd and can be used
>> by many testpmd debug log.
>>
>> Why not create a separate setting for it, perhaps like pkt_verbose?
> 
> The original code in rxonly.c used the verbose level, I don't think it is good idea to add a new pkt_verbose.
> Users will be expecting it to work as before.

In same logic they won't expect verbose=1 only for Rx and verbose=2 only for Tx,
previously this was on/off setting.

since we are touching here already clarifying this with its own config item
makes more sense to me.
And user may not be wanting to have testpmd verbose level 3 and related logs but
only Rx/Tx packets.
Iremonger, Bernard Oct. 11, 2018, 4:39 p.m. UTC | #5 
Hi Ferruh

<snip>
> >> Subject: Re: [dpdk-dev] [PATCH v4 3/3] app/testpmd: set packet dump
> >> based on verbosity level
> >>
> >> On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
> >>> when changing verbosity level it will configure rx/tx callbacks to
> >>> dump packets based on the verbosity value as following:
> >>>     1- dump only received packets:
> >>>        testpmd> set verbose 1
> >>>     2- dump only sent packets:
> >>>        testpmd> set verbose 2
> >>>     3- dump sent and received packets:
> >>>        testpmd> set verbose (any number > 2)
> >>>     4- disable dump
> >>>        testpmd> set verbose 0
> >>
> >> It is good to able to enable Rx/Tx separately but you are overloading
> "verbose"
> >> meaning here. "verbose" is kind of log_level internal to testpmd and
> >> can be used by many testpmd debug log.
> >>
> >> Why not create a separate setting for it, perhaps like pkt_verbose?
> >
> > The original code in rxonly.c used the verbose level, I don't think it is good idea
> to add a new pkt_verbose.
> > Users will be expecting it to work as before.
> 
> In same logic they won't expect verbose=1 only for Rx and verbose=2 only for
> Tx, previously this was on/off setting.
> 
> since we are touching here already clarifying this with its own config item makes
> more sense to me.
> And user may not be wanting to have testpmd verbose level 3 and related logs
> but only Rx/Tx packets.

Previously: 
Set verbose 1, dumped rx packets.
Set verbose 0, disabled dump of rx packets.

Now:
Set verbose 2, dumps tx packets (new functionality, not dumped previously)
Set verbose 3, dumps tx and rx packets (new functionality)

I am ok with this.
I think more opinions are needed.

Regards,

Bernard.
Ferruh Yigit Oct. 17, 2018, 12:34 p.m. UTC | #6 
On 10/11/2018 5:39 PM, Iremonger, Bernard wrote:
> Hi Ferruh
> 
> <snip>
>>>> Subject: Re: [dpdk-dev] [PATCH v4 3/3] app/testpmd: set packet dump
>>>> based on verbosity level
>>>>
>>>> On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
>>>>> when changing verbosity level it will configure rx/tx callbacks to
>>>>> dump packets based on the verbosity value as following:
>>>>>     1- dump only received packets:
>>>>>        testpmd> set verbose 1
>>>>>     2- dump only sent packets:
>>>>>        testpmd> set verbose 2
>>>>>     3- dump sent and received packets:
>>>>>        testpmd> set verbose (any number > 2)
>>>>>     4- disable dump
>>>>>        testpmd> set verbose 0
>>>>
>>>> It is good to able to enable Rx/Tx separately but you are overloading
>> "verbose"
>>>> meaning here. "verbose" is kind of log_level internal to testpmd and
>>>> can be used by many testpmd debug log.
>>>>
>>>> Why not create a separate setting for it, perhaps like pkt_verbose?
>>>
>>> The original code in rxonly.c used the verbose level, I don't think it is good idea
>> to add a new pkt_verbose.
>>> Users will be expecting it to work as before.
>>
>> In same logic they won't expect verbose=1 only for Rx and verbose=2 only for
>> Tx, previously this was on/off setting.
>>
>> since we are touching here already clarifying this with its own config item makes
>> more sense to me.
>> And user may not be wanting to have testpmd verbose level 3 and related logs
>> but only Rx/Tx packets.
> 
> Previously: 
> Set verbose 1, dumped rx packets.
> Set verbose 0, disabled dump of rx packets.
> 
> Now:
> Set verbose 2, dumps tx packets (new functionality, not dumped previously)
> Set verbose 3, dumps tx and rx packets (new functionality)
> 
> I am ok with this.
> I think more opinions are needed.

OK, what I understand was `verbose` is not for just dumping Rx/Tx packets, it is
a log level for testpmd.
But OK it seems it only used for printing packets and only level 0 & 1 used, so
agreed there won't be any issue to use verbose for this purpose.

But Raslan, can you please document this somewhere proper in testpmd
documentation, this information shouldn't be available only commit log.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index fb45fea..f402e04 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -50,6 +50,7 @@ 
 #endif
 #include <rte_gro.h>
 #include <cmdline_parse_etheraddr.h>
+#include <rte_config.h>
 
 #include "testpmd.h"
 
@@ -2963,11 +2964,35 @@  remove_tx_dump_callbacks(portid_t portid)
 }
 
 void
+configure_rxtx_dump_callbacks(uint16_t verbose)
+{
+	portid_t portid;
+
+#ifndef RTE_ETHDEV_RXTX_CALLBACKS
+		TESTPMD_LOG(ERR, "setting rxtx callbacks is not enabled\n");
+		return;
+#endif
+
+	RTE_ETH_FOREACH_DEV(portid)
+	{
+		if (verbose == 1 || verbose > 2)
+			add_rx_dump_callbacks(portid);
+		else
+			remove_rx_dump_callbacks(portid);
+		if (verbose >= 2)
+			add_tx_dump_callbacks(portid);
+		else
+			remove_tx_dump_callbacks(portid);
+	}
+}
+
+void
 set_verbose_level(uint16_t vb_level)
 {
 	printf("Change verbose level from %u to %u\n",
 	       (unsigned int) verbose_level, (unsigned int) vb_level);
 	verbose_level = vb_level;
+	configure_rxtx_dump_callbacks(verbose_level);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 571ecb4..538723c 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1665,7 +1665,7 @@  start_port(portid_t pid)
 					return -1;
 				}
 			}
-
+			configure_rxtx_dump_callbacks(0);
 			printf("Configuring Port %d (socket %u)\n", pi,
 					port->socket_id);
 			/* configure port */
@@ -1764,7 +1764,7 @@  start_port(portid_t pid)
 				return -1;
 			}
 		}
-
+		configure_rxtx_dump_callbacks(verbose_level);
 		/* start port */
 		if (rte_eth_dev_start(pi) < 0) {
 			printf("Fail to start port %d\n", pi);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c0d7656..e68710d 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -757,6 +757,7 @@  void add_rx_dump_callbacks(portid_t portid);
 void remove_rx_dump_callbacks(portid_t portid);
 void add_tx_dump_callbacks(portid_t portid);
 void remove_tx_dump_callbacks(portid_t portid);
+void configure_rxtx_dump_callbacks(uint16_t verbose);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the