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

["Wei Hu (Xavier)" <huwei013@chinasoftinc.com>]

Hi, Ferrh Yigit

On 2020/4/27 22:00, Ferruh Yigit wrote:
> On 4/26/2020 10:22 AM, Wei Hu (Xavier) wrote:
>> 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@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)?
>>>
>>>>
>>>> This patch fixes it by clearing last port statistics when executing
>>>> "clear stats/xstats" command.
>>>>
>>>> Fixes: af75078fece3 ("first public release")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@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.
>>>
>>
>> I will modify it as follows, is it consistent with your comment?
>> Thanks.
>>
>> void
>> fwd_stats_reset(void)
>> {
>> 	streamid_t sm_id;
>> 	portid_t pt_id;
>> 	int i;
>>
>> 	for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
>> 		pt_id = fwd_ports_ids[i];
>> -		rte_eth_stats_get(pt_id, &ports[pt_id].stats);
>> +		rte_eth_stats_reset(port_id);
>> +		meset(&ports[pt_id].stats, 0, sizeof(ports[pt_id].stats));
> 
> No, original code is better. It resets the baseline for forward stats, if you do
> your suggested change it zeros the port stats too which may have
> unexpected/unwanted side affect.
> 
> For consistency I mean the new code you are adding to follow the similar
> approach as existing one (not other-way around :), like:
> 
> nic_xstats_clear(portid_t port_id)
>      ret = rte_eth_xstats_reset(port_id);
>      rte_eth_stats_get(port_id, &ports[port_id].stats);
> 
> but if you prefer to go with 'memset' I guess that is OK too, eventually they
> both give same result although I prefer above one.
> 
Ok, got it.
Thanks.

    Regards
Xavier