From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id CA110A04B1;
	Tue, 29 Sep 2020 15:46:43 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 2B0A61D523;
	Tue, 29 Sep 2020 15:46:41 +0200 (CEST)
Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191])
 by dpdk.org (Postfix) with ESMTP id 2DC891C002;
 Tue, 29 Sep 2020 15:46:38 +0200 (CEST)
Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.58])
 by Forcepoint Email with ESMTP id 515AD5B154F7AD2BABB5;
 Tue, 29 Sep 2020 21:46:36 +0800 (CST)
Received: from [10.67.103.128] (10.67.103.128) by
 DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id
 14.3.487.0; Tue, 29 Sep 2020 21:46:34 +0800
To: Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
 <ferruh.yigit@intel.com>
CC: "techboard@dpdk.org" <techboard@dpdk.org>, <stephen@networkplumber.org>,
 <bruce.richardson@intel.com>, "jerinj@marvell.com" <jerinj@marvell.com>,
 "Ray Kinsella" <mdr@ashroe.eu>, <dev@dpdk.org>
References: <1598845317-55956-1-git-send-email-humin29@huawei.com>
 <baccf0a3-c6a3-5ead-caf1-eb3b34c60401@intel.com>
 <03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com> <4271893.Xu3jSLBPRA@thomas>
From: "Min Hu (Connor)" <humin29@huawei.com>
Message-ID: <e236fc83-e471-17e7-644f-dc256384658c@huawei.com>
Date: Tue, 29 Sep 2020 21:46:35 +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: <4271893.Xu3jSLBPRA@thomas>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-Originating-IP: [10.67.103.128]
X-CFilter-Loop: Reflected
Subject: Re: [dpdk-dev] [dpdk-techboard] [PATCH V5 1/2] dpdk: resolve
 compiling errors for per-queue stats
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi, Thomas,
	I think what you suggest is resonable and helpful.
	But I should have time to fix it about this patch.
	So, this patch will be sent to community in future.
	
	By the way, the other patch is acked by you.
	Acked-by: Thomas Monjalon <thomas@monjalon.net>
	
	This patch is :
	"[V5,2/2] ethdev: change data type in TC rxq and TC txq"
	https://patches.dpdk.org/patch/78888/

	Because we will have National Day Holiday from
	10/1 to 10/8, I will have no time to handle the patch.
	So, I wish that patch can be merged into V20.11 soon.

	Thanks very much.
	
在 2020/9/29 17:33, Thomas Monjalon 写道:
> 29/09/2020 06:49, Min Hu (Connor):
>>
>> 在 2020/9/28 21:53, Ferruh Yigit 写道:
>>> On 9/28/2020 10:16 AM, Thomas Monjalon wrote:
>>>> 28/09/2020 10:59, Ferruh Yigit:
>>>>> On 9/27/2020 4:16 AM, Min Hu (Connor) wrote:
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> Currently, only statistics of rx/tx queues with queue_id less than
>>>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS can be displayed. If there is a certain
>>>>>> application scenario that it needs to use 256 or more than 256 queues
>>>>>> and display all statistics of rx/tx queue. At this moment, we have to
>>>>>> change the macro to be equaled to the queue number.
>>>>>>
>>>>>> However, modifying the macro to be greater than 256 will trigger
>>>>>> many errors and warnings from test-pmd, PMD drivers and librte_ethdev
>>>>>> during compiling dpdk project. But it is possible and permitted that
>>>>>> rx/tx queue number is greater than 256 and all statistics of rx/tx
>>>>>> queue need to be displayed. In addition, the data type of rx/tx queue
>>>>>> number in rte_eth_dev_configure API is 'uint16_t'. So It is
>>>>>> unreasonable
>>>>>> to use the 'uint8_t' type for variables that control which per-queue
>>>>>> statistics can be displayed.
>>>>
>>>> The explanation is too much complex and misleading.
>>>> You mean you cannot increase RTE_ETHDEV_QUEUE_STAT_CNTRS
>>>> above 256 because it is an 8-bit type?
>>>>
>>>> [...]
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>>     int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
>>>>>> -        uint16_t tx_queue_id, uint8_t stat_idx);
>>>>>> +        uint16_t tx_queue_id, uint16_t stat_idx);
>>>> [...]
>>>>>>     int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>>>>>>                            uint16_t rx_queue_id,
>>>>>> -                       uint8_t stat_idx);
>>>>>> +                       uint16_t stat_idx);
>>>> [...]
>>>>> cc'ed tech-board,
>>>>>
>>>>> The patch breaks the ethdev ABI without a deprecation notice from
>>>>> previous
>>>>> release(s).
>>>>>
>>>>> It is mainly a fix to the port_id storage type, which we have updated
>>>>> from
>>>>> uint8_t to uint16_t in past but some seems remained for
>>>>> 'rte_eth_dev_set_tx_queue_stats_mapping()' &
>>>>> 'rte_eth_dev_set_rx_queue_stats_mapping()' APIs.
>>>>
>>>> No, it is not related to the port id, but the number of limited stats.
>>>>
>>>
>>> Right, it is not related to the port id, it is fixing the storage type
>>> for index used to map the queue stats.
>>>
>>>>> Since the ethdev library already heavily breaks the ABI this release,
>>>>> I am for
>>>>> getting this fix, instead of waiting the fix for one more year.
>>>>
>>>> If stats can be managed for more than 256 queues, I think it means
>>>> it is not limited. In this case, we probably don't need the API
>>>> *_queue_stats_mapping which was invented for a limitation of ixgbe.
>>>>
>>>> The problem is probably somewhere else (in testpmd),
>>>> that's why I am against this patch.
>>>>
>>>
>>> This patch is not to fix queue stats mapping, I agree there are problems
>>> related to it, already shared as comment to this set.
>>>
>>> But this patch is to fix the build errors when
>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the
>>> build errors seems around the stats_mapping APIs.
>>
>> Yes, Ferruh  is right.
>>
>> Hi, Thomas,
>> Let me desribe the background again.
>>
>> There exists a certain application scenario: it needs to use 256
>> or more than 256 queues and display all statistics of rx/tx queues.
>> In this scenario, RTE_ETHDEV_QUEUE_STAT_CNTRS should be modifed to
>> 256 or more for the NICs supporting per-queue statistics.
> 
> No, RTE_ETHDEV_QUEUE_STAT_CNTRS does not have to be changed.
> The queue statistics should be retrieved with xstats,
> and abandoned in rte_eth_stats.
> 
> But yes nothing is currently forbidding increasing
> RTE_ETHDEV_QUEUE_STAT_CNTRS, except build issues.
> 
>> While if we did this, compiling codes will cause warnings or errors.
>> WHY? one reson occures in this API:
>> rte_eth_dev_set_tx_queue_stats_mapping(or
>> rte_eth_dev_set_rx_queue_stats_mapping), this is:
>>
>> rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,uint16_t
>> tx_queue_id, uint8_t stat_idx):
>> 	set_queue_stats_mapping:
>> 			...
>> 			if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
>> 			return -EINVAL;
>> 			....
>>
>> As above, stat_idx  is uint8_t, range from 0-255, but
>> RTE_ETHDEV_QUEUE_STAT_CNTRS is 256 (ever larger).
>> So compiling will catch warnings or errors, and failed.
>>
>> So, as Ferruh said,
>> this patch is to fix the build errors when
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the
>> build errors seems around the stats_mapping APIs.
> 
> Really I would prefer deprecating RTE_ETHDEV_QUEUE_STAT_CNTRS,
> instead of increasing size of the mapping API.
> 
>> Really, seting queue stats mapping in testpmd also has some problems
>> that is come up by Ferruh.  In addition, we also modify other
>> unreasonable something about it and we are testing.
>> But it will only modify testpmd, not related to API.
>>
>> Hope for your reply, thanks.
> 
> I understand the issue, you're right raising it,
> but I think the usage of these APIs is wrong.
> I would prefer we work on xstats instead of allowing
> more ugly stuff with legacy stats per queues.
> 
> 2 problems with increasing RTE_ETHDEV_QUEUE_STAT_CNTRS:
> - it makes basic stats huge and slow to retrieve
> - it needs to recompile DPDK
> 
> I proposed a scheme for stats per queue in this presentation:
> https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/WhichStandard.pdf
> 
> If we were at breaking something, we should change the xstats scheme
> from "rx_q%u%s" to "rx_q%u_%s".
> 
> 
> .
>