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 CC2D3A09D3; Thu, 12 Nov 2020 10:52:20 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3441A5928; Thu, 12 Nov 2020 10:52:19 +0100 (CET) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 519411E2B for ; Thu, 12 Nov 2020 10:52:16 +0100 (CET) IronPort-SDR: SWVzv3mJdGBShxxNvWy0jw780T8ovEKqY7td2dPX3Rkviyi/8RTphGfgFbrOLRe3H2gi7KjMZs +m52Xj9IGTCQ== X-IronPort-AV: E=McAfee;i="6000,8403,9802"; a="254992512" X-IronPort-AV: E=Sophos;i="5.77,471,1596524400"; d="scan'208";a="254992512" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2020 01:52:14 -0800 IronPort-SDR: fNOfn4In6mWWMcZSlQq6t7YWLO98uQE9jYbFaiS1qR8ktvE3pE4Nr700/RTuNEpCGGs8h3CAxJ aU+XGwh8x3qw== X-IronPort-AV: E=Sophos;i="5.77,471,1596524400"; d="scan'208";a="542197949" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.194.206]) ([10.213.194.206]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2020 01:52:12 -0800 To: "Min Hu (Connor)" , dev@dpdk.org Cc: bruce.richardson@intel.com, thomas.monjalon@6wind.com, lihuisong@huawei.com References: <1603182389-10087-1-git-send-email-humin29@huawei.com> <1603182389-10087-2-git-send-email-humin29@huawei.com> <34c8c7ac-a799-8624-b530-3c136fd16811@huawei.com> <3f67aa47-7bec-d31c-8162-e800c9800e8a@huawei.com> From: Ferruh Yigit Message-ID: <531504ce-0547-48c2-4e34-c2ed6cb12e57@intel.com> Date: Thu, 12 Nov 2020 09:52:09 +0000 MIME-Version: 1.0 In-Reply-To: <3f67aa47-7bec-d31c-8162-e800c9800e8a@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC V2 1/2] app/testpmd: fix queue stats mapping configuration 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" On 11/12/2020 2:28 AM, Min Hu (Connor) wrote: > Hi Ferruh, any suggestions? > > > 在 2020/11/3 14:30, Min Hu (Connor) 写道: >> Hi Ferruh, >> >> I agree with your proposal. But if we remove record structures, we will >> not be able to query the current queue stats mapping configuration. Or >> we can provide a query API for the PMD driver that uses the >> set_queue_stats_mapping API, and driver records these mapping >> information from user. >> >> What do you think? >> Sorry for delay, Yes that information will be lost, but since the queue stats mapping is not commonly used, I think it can be OK to loose it. As you said another option is to add a new ethdev API to get queue stats mapping, but because of same reason not sure about adding it. We can add it later if there is a request for it. >> >> 在 2020/10/31 4:54, Ferruh Yigit 写道: >>> On 10/20/2020 9:26 AM, Min Hu (Connor) wrote: >>>> From: Huisong Li >>>> >>>> Currently, the queue stats mapping has the following problems: >>>> 1) Many PMD drivers don't support queue stats mapping. But there is no >>>> failure message after executing the command "set stat_qmap rx 0 2 2". >>>> 2) Once queue mapping is set, unrelated and unmapped queues are also >>>> displayed. >>>> 3) There is no need to keep cache line alignment for >>>> 'struct queue_stats_mappings'. >>>> 4) The mapping arrays, 'tx_queue_stats_mappings_array' & >>>> 'rx_queue_stats_mappings_array' are global and their sizes are based on >>>> fixed max port and queue size assumptions. >>>> 5) The configuration result does not take effect or can not be queried >>>> in real time. >>>> >>>> Therefore, we have made the following adjustments: >>>> 1) If PMD supports queue stats mapping, configure to driver in real time >>>> after executing the command "set stat_qmap rx/tx ...". If not, >>>> the command can not be accepted. >>>> 2) Only display queues that mapping done by adding a new 'active' field >>>>   in queue_stats_mappings struct. >>>> 3) Remove cache alignment for 'struct queue_stats_mappings'. >>>> 4) Add a new port_stats_mappings struct in rte_port. >>>> The struct contains number of rx/txq stats mapping, rx/tx >>>> queue_stats_mapping_enabled flag, and rx/tx queue_stats_mapping array. >>>> Size of queue_stats_mapping_array is set to "RTE_ETHDEV_QUEUE_STAT_CNTRS" >>>>   to ensure that the same number of queues can be set for each port. >>>> >>> >>> Hi Connor, >>> >>> I think above adjustment are good, but after the decision to use xstats for >>> the queue stats, what do you think about more simplification, >>> >>> 1) >>> What testpmd does is, records the queue stats mapping commands and registers >>> them later on port start & forwarding start. >>> What happens if recording and registering completely removed? >>> When "set stat_qmap .." issued, it just call the ethdev APIs to do the >>> mapping in device. >>> This lets us removing record structures, "struct port_stats_mappings >>> p_stats_map" >>> Also can remove 'map_port_queue_stats_mapping_registers()' and its sub >>> functions. >>> >>> 2) >>> Also lets remove "tx-queue-stats-mapping" & "rx-queue-stats-mapping" >>> parameters, which enables removing 'parse_queue_stats_mapping_config()' >>> function too >>> >>> 3) >>> Another problem is to display the queue stats, in 'fwd_stats_display()' & >>> 'nic_stats_display()', there is a check if the queue stats mapping enable or >>> not ('rx_queue_stats_mapping_enabled' & 'tx_queue_stats_mapping_enabled'), >>> I think displaying queue stats and queue stat mapping should be separate, why >>> not drop checks for queue stats mapping and display queue stats for 'nb_rxq' >>> & 'nb_txq' queues? >>> >>> Does above make sense? >>> >>> >>> Majority of the drivers doesn't require queue stat mapping to get the queue >>> stats, lets don't pollute main usage with this requirement. >>> >>> >>>> Fixes: 4dccdc789bf4b ("app/testpmd: simplify handling of stats mappings error") >>>> Fixes: 013af9b6b64f6 ("app/testpmd: various updates") >>>> Fixes: ed30d9b691b21 ("app/testpmd: add stats per queue") >>>> >>>> Signed-off-by: Huisong Li >>> >>> <...> >>> >>> .