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 19F3BA0547; Wed, 27 Oct 2021 02:09:12 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A77B440DDA; Wed, 27 Oct 2021 02:09:11 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 5BAE6407FF for ; Wed, 27 Oct 2021 02:09:09 +0200 (CEST) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Hf87b5YDhzbnGc for ; Wed, 27 Oct 2021 08:04:27 +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.15; Wed, 27 Oct 2021 08:09:06 +0800 To: Ferruh Yigit , "Li, Xiaoyun" , "dev@dpdk.org" References: <20210914031233.988-1-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: <29acc2e5-6b26-2ea4-9762-5a1727f7eb2c@huawei.com> Date: Wed, 27 Oct 2021 08:09:06 +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="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) 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" 在 2021/10/27 0:22, Ferruh Yigit 写道: > On 9/17/2021 4:33 AM, Min Hu (Connor) wrote: >> 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 ? >> > > I agree with Xiaoyun on possible race conditions, and to restrict the > secondary > to read the stats but not clear it. > Got it, this patch can be abandoned, thanks. >>> >>> 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 >>> >>> . >>> > > .