From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DD7F1A0C46; Fri, 17 Sep 2021 05:34:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 62063406B4; Fri, 17 Sep 2021 05:34:03 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 982A040689 for ; Fri, 17 Sep 2021 05:34:01 +0200 (CEST) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4H9ffc385hz1DH4R for ; Fri, 17 Sep 2021 11:32:56 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by dggeme756-chm.china.huawei.com (10.3.19.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.8; Fri, 17 Sep 2021 11:33:59 +0800 To: "Li, Xiaoyun" , "dev@dpdk.org" CC: "Yigit, Ferruh" References: <20210914031233.988-1-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: Date: Fri, 17 Sep 2021 11:33:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="gbk"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggeme756-chm.china.huawei.com (10.3.19.102) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix statistics in multiple process X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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, Xiaoyun, ÔÚ 2021/9/16 13:17, Li, Xiaoyun дµÀ: > Hi > >> -----Original Message----- >> From: Min Hu (Connor) >> Sent: Tuesday, September 14, 2021 11:13 >> To: dev@dpdk.org >> Cc: Yigit, Ferruh ; Li, Xiaoyun >> Subject: [PATCH] app/testpmd: fix statistics in multiple process >> >> Currently, 'clear port stats all' in secondary will clear stats in PMD, and also >> update stats which APP stores in 'ports[]' array in secondary process, note that, >> that in primary process remains unchanged. So, when 'show fwd stats all' in >> primary process, stats in PMD may be less than stats which APP stores in 'ports[]' >> array(the stats is different with that in secondary). So forward statistics(stats in >> PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this: >> ---------------------- Forward statistics for port 0 -------------- >> RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766 >> RX-total: 18446744073697645318 >> TX-packets: 18446744073703122216 TX-dropped: 0 >> TX-total: 18446744073703122216 >> -------------------------------------------------------------------- >> Stats in PMD are shared between multiple processes, but stats which APP stores >> have their own copies in multiple processes. And this will result in bugs. >> >> This patch will fix it by creating shared memory to store last stats for multiple >> and secondary process in testpmd. > > Why not just limit "clear port stats " behavior to only primary process? > Is there any particular reason second process has to do clear stats? While, I have no idea if some particular reason exists second process has to do clear stats. I just want to implement this function. > > Stats is quite complicate struct. I feel like it will have race condition issue if you allow two processes to clear it. > Only allow getting for multiple processes makes more sense. Yes, I agree with you, maybe I can add constrains in testpmd guide doc. BTW, what abot any others' opinion ? > > Also your patch has issues even if this idea is accepted by others. Please see below. > >> >> Fixes: 184de26c78d0 ("app/testpmd: support multi-process") >> >> Signed-off-by: Min Hu (Connor) >> --- >> app/test-pmd/config.c | 4 +-- >> app/test-pmd/testpmd.c | 63 ++++++++++++++++++++++++++++++++++++------ >> app/test-pmd/testpmd.h | 2 +- >> 3 files changed, 58 insertions(+), 11 deletions(-) >> > >> @@ -3572,6 +3614,7 @@ init_port_config(void) >> port->dev_conf.intr_conf.lsc = 1; >> if (rmv_interrupt && (*port->dev_info.dev_flags & >> RTE_ETH_DEV_INTR_RMV)) >> port->dev_conf.intr_conf.rmv = 1; >> + port_stats_init(pid); > > Init_port_config is called in several places. Why init stats as 0 for port->stats here? > You did so. Then if user use cmd like "port config 0 rxq 2" which calls init_port_config, port->stats is cleared while device stats isn't. > Then the fwd stats you got will be wrong. > > Please read code carefully. Port->stats will be reset as device stats when start_fwd. Not reset as 0. > > Port->stats should be only cleared when you do clear_nic_stats. Otherwise, it doesn't make sense as "last port stats". > I know your meaning. Actually, My purpose for 'port_stats_init' is only called for all port at init stage. While 'init_port config' confusd me, May I can put the 'port_stats_init' in 'init_config', this may can work out. Thanks. >> } >> } >> >> @@ -3882,6 +3925,10 @@ main(int argc, char** argv) >> rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n", >> rte_strerror(rte_errno)); >> >> + ret = eth_stats_zone_reserve(); >> + if (ret != 0) >> + rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats failed"); >> + >> ret = register_eth_event_callback(); >> if (ret != 0) >> rte_exit(EXIT_FAILURE, "Cannot register for ethdev events"); >> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index >> 5863b2f43f..a48a302343 100644 >> --- a/app/test-pmd/testpmd.h >> +++ b/app/test-pmd/testpmd.h >> @@ -202,7 +202,7 @@ struct rte_port { >> struct rte_eth_dev_info dev_info; /**< PCI info + driver name */ >> struct rte_eth_conf dev_conf; /**< Port configuration. */ >> struct rte_ether_addr eth_addr; /**< Port ethernet address */ >> - struct rte_eth_stats stats; /**< Last port statistics */ >> + struct rte_eth_stats *stats; /**< Last port statistics */ >> unsigned int socket_id; /**< For NUMA support */ >> uint16_t parse_tunnel:1; /**< Parse internal headers */ >> uint16_t tso_segsz; /**< Segmentation offload MSS for non- >> tunneled packets. */ >> -- >> 2.33.0 > > . >