[dpdk-dev] [PATCH] app/testpmd: fix Rx/Tx stats after clear stats command

[Wei Hu (Xavier)]

Hi, Ferruh Yigit

On 2020/4/25 0:12, Ferruh Yigit wrote:
> On 4/24/2020 12:07 PM, Wei Hu (Xavier) wrote:
>> From: Chengwen Feng <fengchengwen at huawei.com>
>>
>> Currently, when running start/clear stats&xstats/stop command many times
>> based on testpmd application, there are incorrect RX/TX-packets stats as
>> below:
>> ---------------------- Forward statistics for port 0  --------------
>> RX-packets: 18446744073709544808 RX-dropped: 0             ...ignore
>> TX-packets: 18446744073709536616 TX-dropped: 0             ...ignore
>> --------------------------------------------------------------------
>>
>> The root cause as below:
>> 1. The struct rte_port of testpmd.h has a member variable
>>     "struct rte_eth_stats stats" to store the last port statistics.
>> 2. When runnig start command, it execute cmd_start_parsed ->
>>     start_packet_forwarding -> fwd_stats_reset, which call rte_eth_stats_get
>>     API function to save current port statistics.
>> 3. When running stop command, it execute fwd_stats_display, which call
>>     rte_eth_stats_get to get current port statistics, and then minus last
>>     port statistics.
>> 4. If we run clear stats or xstats after start command, then run stop,
>>     it may display above incorrect stats because the current Rx/Tx-packets
>>     is lower than the last saved RX/TX-packets(uint64_t overflow).
> 
> Looks like valid issue.
> 
> Can you please update the title to mention this fixes the forward stats (to
> prevent the misunderstanding that issue is in the port stats).
> 
> Also can you please update the documentation
> (doc/guides/testpmd_app_ug/testpmd_funcs.rst), "clear port" command to say this
> will also affect the forward stats output (show fwd)?
> 
   OK, Thanks for your comments.
   I will send V2.

   Regards
Xavier
>>
>> This patch fixes it by clearing last port statistics when executing
>> "clear stats/xstats" command.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable at dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen at huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei at huawei.com>
>> ---
>>   app/test-pmd/config.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 72f25d152..0d2375607 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -234,10 +234,16 @@ nic_stats_display(portid_t port_id)
>>   void
>>   nic_stats_clear(portid_t port_id)
>>   {
>> +	struct rte_port *port;
>> +
>>   	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>>   		print_valid_ports();
>>   		return;
>>   	}
>> +
>> +	port = &ports[port_id];
>> +	/* clear last port statistics because eth stats reset */
>> +	memset(&port->stats, 0, sizeof(port->stats));
> 
> "clear fwd stats" command does same thing in "fwd_stats_reset()" as:
> rte_eth_stats_get(pt_id, &ports[pt_id].stats);
> 
> I suggest doing same here for consistency, but it should be after
> 'rte_eth_stats_reset()' in that case.
> 
>>   	rte_eth_stats_reset(port_id);
>>   	printf("\n  NIC statistics for port %d cleared\n", port_id);
>>   }
>> @@ -308,12 +314,17 @@ nic_xstats_display(portid_t port_id)
>>   void
>>   nic_xstats_clear(portid_t port_id)
>>   {
>> +	struct rte_port *port;
>>   	int ret;
>>   
>>   	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>>   		print_valid_ports();
>>   		return;
>>   	}
>> +
>> +	port = &ports[port_id];
>> +	/* clear last port statistics because eth xstats(include stats) reset */
>> +	memset(&port->stats, 0, sizeof(port->stats));
>>   	ret = rte_eth_xstats_reset(port_id);
>>   	if (ret != 0) {
>>   		printf("%s: Error: failed to reset xstats (port %u): %s",
>>