From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2D23CA00BE; Tue, 28 Apr 2020 10:42:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 335B01D17E; Tue, 28 Apr 2020 10:42:55 +0200 (CEST) Received: from mail.chinasoftinc.com (unknown [114.113.233.8]) by dpdk.org (Postfix) with ESMTP id C7E161C2F9 for ; Tue, 28 Apr 2020 10:42:52 +0200 (CEST) Received: from [192.168.1.199] (139.159.243.11) by INCCAS001.ito.icss (10.168.0.60) with Microsoft SMTP Server id 14.3.487.0; Tue, 28 Apr 2020 16:42:47 +0800 To: Ferruh Yigit , References: <20200424110750.42456-1-huwei013@chinasoftinc.com> <1d83b9c8-a722-48db-18e5-b01a4bb382bb@intel.com> <0d38af77-6ed2-9398-b6fa-720e30070cb8@intel.com> From: "Wei Hu (Xavier)" Message-ID: Date: Tue, 28 Apr 2020 16:42:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <0d38af77-6ed2-9398-b6fa-720e30070cb8@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [139.159.243.11] Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix Rx/Tx stats after clear stats command X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>>> >>>> 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 >>>> Signed-off-by: Wei Hu (Xavier) >>>> --- >>>> 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