DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Wei Hu (Xavier)" <xavier_huwei@163.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix Rx/Tx stats after clear stats command
Date: Mon, 27 Apr 2020 15:00:05 +0100	[thread overview]
Message-ID: <0d38af77-6ed2-9398-b6fa-720e30070cb8@intel.com> (raw)
In-Reply-To: <e2927ac8-a2b4-9619-9f67-d92a0d381dd1@163.com>

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.

  reply	other threads:[~2020-04-27 14:00 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 11:07 Wei Hu (Xavier)
2020-04-24 16:12 ` Ferruh Yigit
2020-04-26  1:36   ` Wei Hu (Xavier)
2020-04-26  1:58     ` Ferruh Yigit
2020-04-26  9:22   ` Wei Hu (Xavier)
2020-04-27 14:00     ` Ferruh Yigit [this message]
2020-04-28  8:42       ` Wei Hu (Xavier)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0d38af77-6ed2-9398-b6fa-720e30070cb8@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=xavier_huwei@163.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).