On 4/26/2020 2:36 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 <fengcheng...@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: sta...@dpdk.org
>>>
>>> Signed-off-by: Chengwen Feng <fengcheng...@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier) <xavier.hu...@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.
What do you think about above comment?
>>
>>> 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",
>>>
