[V2,1/4] ethdev: fix compiling errors for per-queue statistics
Checks
Commit Message
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 driver and librte_ethdev
during compiling dpdk project. But it is possible and permited 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.
Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
Fixes: e6defdfddc3b ("net/igc: enable statistics")
Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
---
app/proc-info/main.c | 2 +-
app/test-pmd/cmdline.c | 2 +-
app/test-pmd/config.c | 4 ++--
app/test-pmd/testpmd.c | 2 +-
app/test-pmd/testpmd.h | 5 +++--
drivers/net/igc/igc_ethdev.c | 4 ++--
drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++--
drivers/net/memif/rte_eth_memif.c | 2 +-
drivers/net/octeontx2/otx2_ethdev.h | 2 +-
drivers/net/octeontx2/otx2_stats.c | 2 +-
drivers/net/virtio/virtio_ethdev.c | 4 ++--
lib/librte_ethdev/rte_ethdev.c | 6 +++---
lib/librte_ethdev/rte_ethdev.h | 4 ++--
lib/librte_ethdev/rte_ethdev_core.h | 2 +-
14 files changed, 23 insertions(+), 22 deletions(-)
Comments
On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
> during compiling dpdk project. But it is possible and permited 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.
>
> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
> Fixes: e6defdfddc3b ("net/igc: enable statistics")
> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
> app/proc-info/main.c | 2 +-
> app/test-pmd/cmdline.c | 2 +-
> app/test-pmd/config.c | 4 ++--
> app/test-pmd/testpmd.c | 2 +-
> app/test-pmd/testpmd.h | 5 +++--
> drivers/net/igc/igc_ethdev.c | 4 ++--
> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++--
> drivers/net/memif/rte_eth_memif.c | 2 +-
> drivers/net/octeontx2/otx2_ethdev.h | 2 +-
> drivers/net/octeontx2/otx2_stats.c | 2 +-
> drivers/net/virtio/virtio_ethdev.c | 4 ++--
> lib/librte_ethdev/rte_ethdev.c | 6 +++---
> lib/librte_ethdev/rte_ethdev.h | 4 ++--
> lib/librte_ethdev/rte_ethdev_core.h | 2 +-
> 14 files changed, 23 insertions(+), 22 deletions(-)
<...>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0a6ed85..40b6b17 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -8244,7 +8244,7 @@ struct cmd_set_qmap_result {
> cmdline_fixed_string_t what;
> portid_t port_id;
> uint16_t queue_id;
> - uint8_t map_value;
> + uint16_t map_value;
> };
'cmd_setqmap_mapvalue' should be 'UINT16' instead of 'UINT8', or can't set
'map_value' >= 256 via "set stat_qmap ..." command.
On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
> during compiling dpdk project. But it is possible and permited 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.
>
> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
> Fixes: e6defdfddc3b ("net/igc: enable statistics")
> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
The patch mostly looks good and it enables build with
'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
Only I put a comment for a testpmd change to enable the "set stat_qmap" command
for map value > 256.
BUT there are many things to fix in the queue stats mapping, since you are
already on it can you help on a few things on testpmd related to it, if you have
time for it?
1) Getting queue stats shouldn't require stats mapping, it should be controlled
separately. Many PMDs doesn't require/do the stats mapping but they still can
collect the per queue stats, which can be displayed independent from mapping.
2) Even you map only one queue, while displaying stats it will display
'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high it makes hard
to see the actual interested values.
If there is no mapping, it should display min(number_of_queues,
RTE_ETHDEV_QUEUE_STAT_CNTRS).
If there is mapping it should display queues that mapping done, this may require
adding a new 'active' field to 'struct queue_stats_mappings'.
3) Why 'struct queue_stats_mappings' is cache aligned, is it really needed?
4) The mapping arrays, 'tx_queue_stats_mappings_array' &
'rx_queue_stats_mappings_array' are global and their size is based on fixed max
port and queue size assumptions, can those mapping array be done per port and
'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
Thanks,
ferruh
04/09/2020 20:31, Ferruh Yigit:
> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
> > during compiling dpdk project. But it is possible and permited 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.
> >
> > Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
> > Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> > Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
> > Fixes: e6defdfddc3b ("net/igc: enable statistics")
> > Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
> > Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Huisong Li <lihuisong@huawei.com>
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> > Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>
> The patch mostly looks good and it enables build with
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
> Only I put a comment for a testpmd change to enable the "set stat_qmap" command
> for map value > 256.
>
>
> BUT there are many things to fix in the queue stats mapping, since you are
> already on it can you help on a few things on testpmd related to it, if you have
> time for it?
>
> 1) Getting queue stats shouldn't require stats mapping, it should be controlled
> separately. Many PMDs doesn't require/do the stats mapping but they still can
> collect the per queue stats, which can be displayed independent from mapping.
>
> 2) Even you map only one queue, while displaying stats it will display
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high it makes hard
> to see the actual interested values.
> If there is no mapping, it should display min(number_of_queues,
> RTE_ETHDEV_QUEUE_STAT_CNTRS).
> If there is mapping it should display queues that mapping done, this may require
> adding a new 'active' field to 'struct queue_stats_mappings'.
>
> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really needed?
>
> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
> 'rx_queue_stats_mappings_array' are global and their size is based on fixed max
> port and queue size assumptions, can those mapping array be done per port and
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
Yes there are a lot of things to review in this area.
For reference, the slides I presented last year:
https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/WhichStandard.pdf
I think we should fix the xstats name "rx_q%u%s" to "rx_q%u_%s".
在 2020/9/5 2:31, Ferruh Yigit 写道:
> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
>> during compiling dpdk project. But it is possible and permited 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.
>>
>> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
>> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
>> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
>> Fixes: e6defdfddc3b ("net/igc: enable statistics")
>> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
>> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>
> The patch mostly looks good and it enables build with
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
> Only I put a comment for a testpmd change to enable the "set stat_qmap" command
> for map value > 256.
>
>
> BUT there are many things to fix in the queue stats mapping, since you are
> already on it can you help on a few things on testpmd related to it, if you have
> time for it?
>
> 1) Getting queue stats shouldn't require stats mapping, it should be controlled
> separately. Many PMDs doesn't require/do the stats mapping but they still can
> collect the per queue stats, which can be displayed independent from mapping.
>
> 2) Even you map only one queue, while displaying stats it will display
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high it makes hard
> to see the actual interested values.
> If there is no mapping, it should display min(number_of_queues,
> RTE_ETHDEV_QUEUE_STAT_CNTRS).
> If there is mapping it should display queues that mapping done, this may require
> adding a new 'active' field to 'struct queue_stats_mappings'.
>
> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really needed?
>
> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
> 'rx_queue_stats_mappings_array' are global and their size is based on fixed max
> port and queue size assumptions, can those mapping array be done per port and
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
>
It does seem to be unreasonable. We also try do it, and found that
it is hard to control per queue stats and queue stats mapping, separately.
For details, see next V3 patch.
We're working on debugging this stats_mapping setting. During this process,
we have a problem that starting testpmd with --rx/tx-queue-stats-mapping
parameter fails. log as follows:
[root]$ ./testpmd -l 1,3,4,8,12 -n 4 -w 0000:04:00.0 --file-prefix=lee
--log-level=7
-- -i --rxq=6 --txq=6 --burst=64 --rxd=2048 --txd=2048 --nb-cores=4
--rx-queue-stats-mapping=(0,2,2) --tx-queue-stats-mapping=(0,3,3)
-bash: syntax error near unexpected token `('
[root]$
thanks.
> Thanks,
> ferruh
>
>
> .
>
On 9/23/2020 3:31 AM, Min Hu (Connor) wrote:
>
>
> 在 2020/9/5 2:31, Ferruh Yigit 写道:
>> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
>>> during compiling dpdk project. But it is possible and permited 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.
>>>
>>> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
>>> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
>>> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
>>> Fixes: e6defdfddc3b ("net/igc: enable statistics")
>>> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
>>> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>
>> The patch mostly looks good and it enables build with
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
>> Only I put a comment for a testpmd change to enable the "set
>> stat_qmap" command
>> for map value > 256.
>>
>>
>> BUT there are many things to fix in the queue stats mapping, since you
>> are
>> already on it can you help on a few things on testpmd related to it,
>> if you have
>> time for it?
>>
>> 1) Getting queue stats shouldn't require stats mapping, it should be
>> controlled
>> separately. Many PMDs doesn't require/do the stats mapping but they
>> still can
>> collect the per queue stats, which can be displayed independent from
>> mapping.
>>
>> 2) Even you map only one queue, while displaying stats it will display
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high it
>> makes hard
>> to see the actual interested values.
>> If there is no mapping, it should display min(number_of_queues,
>> RTE_ETHDEV_QUEUE_STAT_CNTRS).
>> If there is mapping it should display queues that mapping done, this
>> may require
>> adding a new 'active' field to 'struct queue_stats_mappings'.
>>
>> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really
>> needed?
>>
>> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
>> 'rx_queue_stats_mappings_array' are global and their size is based on
>> fixed max
>> port and queue size assumptions, can those mapping array be done per
>> port and
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
>>
> It does seem to be unreasonable. We also try do it, and found that
> it is hard to control per queue stats and queue stats mapping, separately.
> For details, see next V3 patch.
I will check v3, but as far as know stats mapping exist because of
limitations of some NICs, that there are N queues but M stats registers
where N > M. So need to map some queues to stat registers, and there is
N:1 relation there, so multiple queue stats can be represented in single
stat register.
But for rest of the NICs it should be possible to display the queue
stats independent from the mapping.
> We're working on debugging this stats_mapping setting. During this process,
> we have a problem that starting testpmd with --rx/tx-queue-stats-mapping
> parameter fails. log as follows:
> [root]$ ./testpmd -l 1,3,4,8,12 -n 4 -w 0000:04:00.0 --file-prefix=lee
> --log-level=7
> -- -i --rxq=6 --txq=6 --burst=64 --rxd=2048 --txd=2048 --nb-cores=4
> --rx-queue-stats-mapping=(0,2,2) --tx-queue-stats-mapping=(0,3,3)
>
> -bash: syntax error near unexpected token `('
> [root]$
Above seems bash error, using '' can help for it, like
--rx-queue-stats-mapping='(0,2,2)'
Hi, Ferruh Yigit,
Your method is very practical. Thanks!
But we found that testpmd fails to start with "--rx-queue-stats-mapping"
and "--tx-queue-stats-mapping". We've found the cause and modified it.
These modifications are being tested.
Currently, the following modifications are related to framework APIs.
1)struct 'rte_eth_dcb_tc_queue_mapping'
2)fix 'rte_eth_dev_set_rx/tx_queue_stats_mapping' function to resolve
the problem that DPDK project fails to build when
'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
According to my current analysis, these modifications for queue stats
mapping in testpmd do not involve the modification of framework APIs.
Therefore, I think we should give priority to focusing on the current
changes to the DPDK framework APIs.
After all, DPDK-20.11 is approaching. What do you think?
在 2020/9/23 17:18, Ferruh Yigit 写道:
> On 9/23/2020 3:31 AM, Min Hu (Connor) wrote:
>>
>>
>> 在 2020/9/5 2:31, Ferruh Yigit 写道:
>>> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
>>>> during compiling dpdk project. But it is possible and permited 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.
>>>>
>>>> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
>>>> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
>>>> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
>>>> Fixes: e6defdfddc3b ("net/igc: enable statistics")
>>>> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
>>>> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>
>>> The patch mostly looks good and it enables build with
>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
>>> Only I put a comment for a testpmd change to enable the "set
>>> stat_qmap" command
>>> for map value > 256.
>>>
>>>
>>> BUT there are many things to fix in the queue stats mapping, since
>>> you are
>>> already on it can you help on a few things on testpmd related to it,
>>> if you have
>>> time for it?
>>>
>>> 1) Getting queue stats shouldn't require stats mapping, it should be
>>> controlled
>>> separately. Many PMDs doesn't require/do the stats mapping but they
>>> still can
>>> collect the per queue stats, which can be displayed independent from
>>> mapping.
>>>
>>> 2) Even you map only one queue, while displaying stats it will display
>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high it
>>> makes hard
>>> to see the actual interested values.
>>> If there is no mapping, it should display min(number_of_queues,
>>> RTE_ETHDEV_QUEUE_STAT_CNTRS).
>>> If there is mapping it should display queues that mapping done, this
>>> may require
>>> adding a new 'active' field to 'struct queue_stats_mappings'.
>>>
>>> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really
>>> needed?
>>>
>>> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
>>> 'rx_queue_stats_mappings_array' are global and their size is based on
>>> fixed max
>>> port and queue size assumptions, can those mapping array be done per
>>> port and
>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
>>>
>> It does seem to be unreasonable. We also try do it, and found that
>> it is hard to control per queue stats and queue stats mapping,
>> separately.
>> For details, see next V3 patch.
>
> I will check v3, but as far as know stats mapping exist because of
> limitations of some NICs, that there are N queues but M stats registers
> where N > M. So need to map some queues to stat registers, and there is
> N:1 relation there, so multiple queue stats can be represented in single
> stat register.
>
> But for rest of the NICs it should be possible to display the queue
> stats independent from the mapping.
>
>
>> We're working on debugging this stats_mapping setting. During this
>> process,
>> we have a problem that starting testpmd with --rx/tx-queue-stats-mapping
>> parameter fails. log as follows:
>> [root]$ ./testpmd -l 1,3,4,8,12 -n 4 -w 0000:04:00.0 --file-prefix=lee
>> --log-level=7
>> -- -i --rxq=6 --txq=6 --burst=64 --rxd=2048 --txd=2048 --nb-cores=4
>> --rx-queue-stats-mapping=(0,2,2) --tx-queue-stats-mapping=(0,3,3)
>>
>> -bash: syntax error near unexpected token `('
>> [root]$
>
> Above seems bash error, using '' can help for it, like
> --rx-queue-stats-mapping='(0,2,2)'
> .
On 9/25/2020 9:58 AM, Min Hu (Connor) wrote:
> Hi, Ferruh Yigit,
>
> Your method is very practical. Thanks!
> But we found that testpmd fails to start with "--rx-queue-stats-mapping"
> and "--tx-queue-stats-mapping". We've found the cause and modified it.
> These modifications are being tested.
>
> Currently, the following modifications are related to framework APIs.
> 1)struct 'rte_eth_dcb_tc_queue_mapping'
> 2)fix 'rte_eth_dev_set_rx/tx_queue_stats_mapping' function to resolve
> the problem that DPDK project fails to build when
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
>
> According to my current analysis, these modifications for queue stats
> mapping in testpmd do not involve the modification of framework APIs.
> Therefore, I think we should give priority to focusing on the current
> changes to the DPDK framework APIs.
>
> After all, DPDK-20.11 is approaching. What do you think?
>
Makes sense, agree to continue with this patchset and queue stats
mapping related changes can be done later.
>
> 在 2020/9/23 17:18, Ferruh Yigit 写道:
>> On 9/23/2020 3:31 AM, Min Hu (Connor) wrote:
>>>
>>>
>>> 在 2020/9/5 2:31, Ferruh Yigit 写道:
>>>> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
>>>>> during compiling dpdk project. But it is possible and permited 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.
>>>>>
>>>>> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
>>>>> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
>>>>> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
>>>>> Fixes: e6defdfddc3b ("net/igc: enable statistics")
>>>>> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
>>>>> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>
>>>> The patch mostly looks good and it enables build with
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
>>>> Only I put a comment for a testpmd change to enable the "set
>>>> stat_qmap" command
>>>> for map value > 256.
>>>>
>>>>
>>>> BUT there are many things to fix in the queue stats mapping, since
>>>> you are
>>>> already on it can you help on a few things on testpmd related to it,
>>>> if you have
>>>> time for it?
>>>>
>>>> 1) Getting queue stats shouldn't require stats mapping, it should be
>>>> controlled
>>>> separately. Many PMDs doesn't require/do the stats mapping but they
>>>> still can
>>>> collect the per queue stats, which can be displayed independent from
>>>> mapping.
>>>>
>>>> 2) Even you map only one queue, while displaying stats it will display
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high
>>>> it makes hard
>>>> to see the actual interested values.
>>>> If there is no mapping, it should display min(number_of_queues,
>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS).
>>>> If there is mapping it should display queues that mapping done, this
>>>> may require
>>>> adding a new 'active' field to 'struct queue_stats_mappings'.
>>>>
>>>> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really
>>>> needed?
>>>>
>>>> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
>>>> 'rx_queue_stats_mappings_array' are global and their size is based
>>>> on fixed max
>>>> port and queue size assumptions, can those mapping array be done per
>>>> port and
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
>>>>
>>> It does seem to be unreasonable. We also try do it, and found that
>>> it is hard to control per queue stats and queue stats mapping,
>>> separately.
>>> For details, see next V3 patch.
>>
>> I will check v3, but as far as know stats mapping exist because of
>> limitations of some NICs, that there are N queues but M stats
>> registers where N > M. So need to map some queues to stat registers,
>> and there is N:1 relation there, so multiple queue stats can be
>> represented in single stat register.
>>
>> But for rest of the NICs it should be possible to display the queue
>> stats independent from the mapping.
>>
>>
>>> We're working on debugging this stats_mapping setting. During this
>>> process,
>>> we have a problem that starting testpmd with --rx/tx-queue-stats-mapping
>>> parameter fails. log as follows:
>>> [root]$ ./testpmd -l 1,3,4,8,12 -n 4 -w 0000:04:00.0
>>> --file-prefix=lee --log-level=7
>>> -- -i --rxq=6 --txq=6 --burst=64 --rxd=2048 --txd=2048 --nb-cores=4
>>> --rx-queue-stats-mapping=(0,2,2) --tx-queue-stats-mapping=(0,3,3)
>>>
>>> -bash: syntax error near unexpected token `('
>>> [root]$
>>
>> Above seems bash error, using '' can help for it, like
>> --rx-queue-stats-mapping='(0,2,2)'
>> .
@@ -348,7 +348,7 @@ static void
nic_stats_display(uint16_t port_id)
{
struct rte_eth_stats stats;
- uint8_t i;
+ uint16_t i;
static const char *nic_stats_border = "########################";
@@ -8244,7 +8244,7 @@ struct cmd_set_qmap_result {
cmdline_fixed_string_t what;
portid_t port_id;
uint16_t queue_id;
- uint8_t map_value;
+ uint16_t map_value;
};
static void
@@ -160,7 +160,7 @@ nic_stats_display(portid_t port_id)
uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
struct rte_eth_stats stats;
struct rte_port *port = &ports[port_id];
- uint8_t i;
+ uint16_t i;
static const char *nic_stats_border = "########################";
@@ -3623,7 +3623,7 @@ tx_vlan_pvid_set(portid_t port_id, uint16_t vlan_id, int on)
}
void
-set_qmap(portid_t port_id, uint8_t is_rx, uint16_t queue_id, uint8_t map_value)
+set_qmap(portid_t port_id, uint8_t is_rx, uint16_t queue_id, uint16_t map_value)
{
uint16_t i;
uint8_t existing_mapping_found = 0;
@@ -1835,7 +1835,7 @@ fwd_stats_display(void)
#endif
}
for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
- uint8_t j;
+ uint16_t j;
pt_id = fwd_ports_ids[i];
port = &ports[pt_id];
@@ -285,7 +285,7 @@ enum dcb_mode_enable
struct queue_stats_mappings {
portid_t port_id;
uint16_t queue_id;
- uint8_t stats_counter_id;
+ uint16_t stats_counter_id;
} __rte_cache_aligned;
extern struct queue_stats_mappings tx_queue_stats_mappings_array[];
@@ -768,7 +768,8 @@ void tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer);
void tx_vlan_reset(portid_t port_id);
void tx_vlan_pvid_set(portid_t port_id, uint16_t vlan_id, int on);
-void set_qmap(portid_t port_id, uint8_t is_rx, uint16_t queue_id, uint8_t map_value);
+void set_qmap(portid_t port_id, uint8_t is_rx, uint16_t queue_id,
+ uint16_t map_value);
void set_xstats_hide_zero(uint8_t on_off);
@@ -221,7 +221,7 @@ static int eth_igc_xstats_get_names_by_id(struct rte_eth_dev *dev,
static int eth_igc_xstats_reset(struct rte_eth_dev *dev);
static int
eth_igc_queue_stats_mapping_set(struct rte_eth_dev *dev,
- uint16_t queue_id, uint8_t stat_idx, uint8_t is_rx);
+ uint16_t queue_id, uint16_t stat_idx, uint8_t is_rx);
static int
eth_igc_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id);
static int
@@ -2076,7 +2076,7 @@ eth_igc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
static int
eth_igc_queue_stats_mapping_set(struct rte_eth_dev *dev,
- uint16_t queue_id, uint8_t stat_idx, uint8_t is_rx)
+ uint16_t queue_id, uint16_t stat_idx, uint8_t is_rx)
{
struct igc_adapter *igc = IGC_DEV_PRIVATE(dev);
@@ -178,7 +178,7 @@ static int ixgbe_dev_xstats_get_names_by_id(
unsigned int limit);
static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
uint16_t queue_id,
- uint8_t stat_idx,
+ uint16_t stat_idx,
uint8_t is_rx);
static int ixgbe_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
size_t fw_size);
@@ -897,7 +897,7 @@ ixgbe_reset_qstat_mappings(struct ixgbe_hw *hw)
static int
ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
uint16_t queue_id,
- uint8_t stat_idx,
+ uint16_t stat_idx,
uint8_t is_rx)
{
#define QSM_REG_NB_BITS_PER_QMAP_FIELD 8
@@ -1341,7 +1341,7 @@ memif_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
struct pmd_internals *pmd = dev->data->dev_private;
struct memif_queue *mq;
int i;
- uint8_t tmp, nq;
+ uint16_t tmp, nq;
stats->ipackets = 0;
stats->ibytes = 0;
@@ -476,7 +476,7 @@ int otx2_nix_dev_stats_get(struct rte_eth_dev *eth_dev,
int otx2_nix_dev_stats_reset(struct rte_eth_dev *eth_dev);
int otx2_nix_queue_stats_mapping(struct rte_eth_dev *dev,
- uint16_t queue_id, uint8_t stat_idx,
+ uint16_t queue_id, uint16_t stat_idx,
uint8_t is_rx);
int otx2_nix_xstats_get(struct rte_eth_dev *eth_dev,
struct rte_eth_xstat *xstats, unsigned int n);
@@ -145,7 +145,7 @@ otx2_nix_dev_stats_reset(struct rte_eth_dev *eth_dev)
int
otx2_nix_queue_stats_mapping(struct rte_eth_dev *eth_dev, uint16_t queue_id,
- uint8_t stat_idx, uint8_t is_rx)
+ uint16_t stat_idx, uint8_t is_rx)
{
struct otx2_eth_dev *dev = otx2_eth_pmd_priv(eth_dev);
@@ -82,7 +82,7 @@ static int virtio_intr_disable(struct rte_eth_dev *dev);
static int virtio_dev_queue_stats_mapping_set(
struct rte_eth_dev *eth_dev,
uint16_t queue_id,
- uint8_t stat_idx,
+ uint16_t stat_idx,
uint8_t is_rx);
static void virtio_notify_peers(struct rte_eth_dev *dev);
@@ -2647,7 +2647,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
*/
static int
virtio_dev_queue_stats_mapping_set(__rte_unused struct rte_eth_dev *eth_dev,
-__rte_unused uint16_t queue_id, __rte_unused uint8_t stat_idx,
+__rte_unused uint16_t queue_id, __rte_unused uint16_t stat_idx,
__rte_unused uint8_t is_rx)
{
return 0;
@@ -2905,7 +2905,7 @@ rte_eth_xstats_reset(uint16_t port_id)
}
static int
-set_queue_stats_mapping(uint16_t port_id, uint16_t queue_id, uint8_t stat_idx,
+set_queue_stats_mapping(uint16_t port_id, uint16_t queue_id, uint16_t stat_idx,
uint8_t is_rx)
{
struct rte_eth_dev *dev;
@@ -2932,7 +2932,7 @@ set_queue_stats_mapping(uint16_t port_id, uint16_t queue_id, uint8_t stat_idx,
int
rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, uint16_t tx_queue_id,
- uint8_t stat_idx)
+ uint16_t stat_idx)
{
return eth_err(port_id, set_queue_stats_mapping(port_id, tx_queue_id,
stat_idx, STAT_QMAP_TX));
@@ -2941,7 +2941,7 @@ rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, uint16_t tx_queue_id,
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)
{
return eth_err(port_id, set_queue_stats_mapping(port_id, rx_queue_id,
stat_idx, STAT_QMAP_RX));
@@ -2610,7 +2610,7 @@ int rte_eth_xstats_reset(uint16_t port_id);
* Zero if successful. Non-zero otherwise.
*/
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);
/**
* Set a mapping for the specified receive queue to the specified per-queue
@@ -2631,7 +2631,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
*/
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);
/**
* Retrieve the Ethernet address of an Ethernet device.
@@ -223,7 +223,7 @@ typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
uint16_t queue_id,
- uint8_t stat_idx,
+ uint16_t stat_idx,
uint8_t is_rx);
/**< @internal Set a queue statistics mapping for a tx/rx queue of an Ethernet device. */