[v4,3/3] app/testpmd: set packet dump based on verbosity level
Checks
Commit Message
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
> -----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>
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?
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.
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.
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.
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.
@@ -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
@@ -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);
@@ -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