DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Min Hu (Connor)" <humin29@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
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>
Subject: Re: [dpdk-dev] [dpdk-techboard] [PATCH V5 1/2] dpdk: resolve compiling errors for per-queue stats
Date: Tue, 29 Sep 2020 12:49:20 +0800	[thread overview]
Message-ID: <03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com> (raw)
In-Reply-To: <baccf0a3-c6a3-5ead-caf1-eb3b34c60401@intel.com>



在 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.

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, 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.






  parent reply	other threads:[~2020-09-29  4:49 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-31  3:41 [dpdk-dev] [RFC 1/2] Description: lib/ethdev: change data type in tc_rxq and tc_txq Min Hu(Connor)
2020-08-31  3:41 ` [dpdk-dev] [RFC 2/2] Description: dpdk: fix compiling errors for more than 256 per-queue statistics Min Hu(Connor)
2020-09-01  1:33 ` [dpdk-dev] [RFC 1/2] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-01  1:33   ` [dpdk-dev] [RFC 2/2] ethdev: fix compiling errors for per-queue statistics Min Hu (Connor)
2020-09-01  2:23     ` Stephen Hemminger
2020-09-01 11:52   ` [dpdk-dev] [PATCH 0/4] ethdev: change the queue ID type Min Hu (Connor)
2020-09-01 11:52     ` [dpdk-dev] [PATCH 1/4] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-04 11:32       ` [dpdk-dev] [PATCH V2 0/4] ethdev: change the queue ID type Min Hu (Connor)
2020-09-04 11:32         ` [dpdk-dev] [PATCH V2 1/4] ethdev: fix compiling errors for per-queue statistics Min Hu (Connor)
2020-09-04 18:04           ` Ferruh Yigit
2020-09-04 18:31           ` Ferruh Yigit
2020-09-05 16:51             ` Thomas Monjalon
2020-09-23  2:31             ` Min Hu (Connor)
2020-09-23  9:18               ` Ferruh Yigit
2020-09-25  8:58                 ` Min Hu (Connor)
2020-09-25  9:36                   ` Ferruh Yigit
2020-09-23 12:59           ` [dpdk-dev] [PATCH V3 0/4] change data type in TC queue Min Hu (Connor)
2020-09-23 12:59             ` [dpdk-dev] [PATCH V3 1/4] dpdk: fix compiling errors for per-queue statistics Min Hu (Connor)
2020-09-23 13:00             ` [dpdk-dev] [PATCH V3 2/4] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-23 13:00             ` [dpdk-dev] [PATCH V3 3/4] doc: announce modified field in DCB TC queue mapping Min Hu (Connor)
2020-09-23 13:00             ` [dpdk-dev] [PATCH V3 4/4] doc: announce modified in queue stats mapping API Min Hu (Connor)
2020-09-25  9:41             ` [dpdk-dev] [PATCH V3 0/4] change data type in TC queue Ferruh Yigit
2020-09-04 11:32         ` [dpdk-dev] [PATCH V2 2/4] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-04 11:32         ` [dpdk-dev] [PATCH V2 3/4] doc: announce modified field in DCB TC queue mapping Min Hu (Connor)
2020-09-04 11:32         ` [dpdk-dev] [PATCH V2 4/4] doc: announce modified field in ethdev API Min Hu (Connor)
2020-09-04 14:33         ` [dpdk-dev] [PATCH V2 0/4] ethdev: change the queue ID type Ferruh Yigit
2020-09-09 12:36       ` [dpdk-dev] [PATCH V3 0/3] add FEC support Min Hu (Connor)
2020-09-09 12:36         ` [dpdk-dev] [PATCH V3 1/3] ethdev: introduce FEC API Min Hu (Connor)
2020-09-09 12:36         ` [dpdk-dev] [PATCH V3 2/3] net/hns3: support FEC Min Hu (Connor)
2020-09-09 12:36         ` [dpdk-dev] [PATCH V3 3/3] app/testpmd: add FEC command Min Hu (Connor)
2020-09-01 11:52     ` [dpdk-dev] [PATCH 2/4] ethdev: fix compiling errors for per-queue statistics Min Hu (Connor)
2020-09-01 16:17       ` Ferruh Yigit
2020-09-01 11:52     ` [dpdk-dev] [PATCH 3/4] doc: announce modified field in DCB TC queue mapping Min Hu (Connor)
2020-09-01 11:52     ` [dpdk-dev] [PATCH 4/4] doc: announce modified field in ethdev API Min Hu (Connor)
2020-09-01 16:14     ` [dpdk-dev] [PATCH 0/4] ethdev: change the queue ID type Ferruh Yigit
2020-09-25 12:51 ` [dpdk-dev] [PATCH V4 0/2] change data type in TC queue Min Hu (Connor)
2020-09-25 12:51   ` [dpdk-dev] [PATCH V4 1/2] dpdk: resolve compiling errors for per-queue stats Min Hu (Connor)
2020-09-25 12:51   ` [dpdk-dev] [PATCH V4 2/2] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-25 13:14   ` [dpdk-dev] [PATCH V4 0/2] change data type in TC queue Ferruh Yigit
2020-09-27  3:16 ` [dpdk-dev] [PATCH V5 " Min Hu (Connor)
2020-09-27  3:16   ` [dpdk-dev] [PATCH V5 1/2] dpdk: resolve compiling errors for per-queue stats Min Hu (Connor)
2020-09-28  8:59     ` Ferruh Yigit
2020-09-28  9:16       ` [dpdk-dev] [dpdk-techboard] " Thomas Monjalon
2020-09-28 12:00         ` Ananyev, Konstantin
2020-09-28 13:47         ` Min Hu (Connor)
2020-09-28 15:35           ` Thomas Monjalon
2020-09-28 13:53         ` Ferruh Yigit
2020-09-28 15:24           ` Thomas Monjalon
2020-09-28 15:43             ` Stephen Hemminger
2020-10-05 12:23               ` Ferruh Yigit
2020-10-06  8:33                 ` Olivier Matz
2020-10-09 20:32                   ` Ferruh Yigit
2020-10-10  8:09                     ` Thomas Monjalon
2020-10-12 17:02                       ` Ferruh Yigit
2020-09-29  4:49           ` Min Hu (Connor) [this message]
2020-09-29  9:33             ` Thomas Monjalon
2020-09-29 13:46               ` Min Hu (Connor)
2020-09-28 11:52       ` Ananyev, Konstantin
2020-09-30  8:34       ` [dpdk-dev] " Kinsella, Ray
2020-09-27  3:16   ` [dpdk-dev] [PATCH V5 2/2] ethdev: change data type in TC rxq and TC txq Min Hu (Connor)
2020-09-28  9:04     ` Ferruh Yigit
2020-09-28  9:21       ` [dpdk-dev] [dpdk-techboard] " Thomas Monjalon
2020-10-05 12:26         ` Ferruh Yigit
2020-10-06 12:04           ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com \
    --to=humin29@huawei.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=mdr@ashroe.eu \
    --cc=stephen@networkplumber.org \
    --cc=techboard@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).