[V5,1/2] dpdk: resolve compiling errors for per-queue stats
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 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.
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/")
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>
---
V4 -> V5:
add release notes updated.
---
v3->v4:
add a change in cmd_setqmap_mapvalue.
---
v2->v3:
change 'uint8_t i' to 'uint16_t i' in nic_stats_display function.
---
app/proc-info/main.c | 2 +-
app/test-pmd/cmdline.c | 4 ++--
app/test-pmd/config.c | 4 ++--
app/test-pmd/testpmd.c | 2 +-
app/test-pmd/testpmd.h | 5 +++--
doc/guides/rel_notes/release_20_11.rst | 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_driver.h | 2 +-
15 files changed, 29 insertions(+), 23 deletions(-)
Comments
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.
>
> 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/")
>
> 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>
> ---
> V4 -> V5:
> add release notes updated.
>
> ---
> v3->v4:
> add a change in cmd_setqmap_mapvalue.
>
> ---
> v2->v3:
> change 'uint8_t i' to 'uint16_t i' in nic_stats_display function.
>
> ---
> app/proc-info/main.c | 2 +-
> app/test-pmd/cmdline.c | 4 ++--
> app/test-pmd/config.c | 4 ++--
> app/test-pmd/testpmd.c | 2 +-
> app/test-pmd/testpmd.h | 5 +++--
> doc/guides/rel_notes/release_20_11.rst | 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_driver.h | 2 +-
> 15 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> index 64fb83b..26d9355 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -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 = "########################";
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 08e123f..23e624f 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -8315,7 +8315,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
> @@ -8346,7 +8346,7 @@ cmdline_parse_token_num_t cmd_setqmap_queueid =
> queue_id, UINT16);
> cmdline_parse_token_num_t cmd_setqmap_mapvalue =
> TOKEN_NUM_INITIALIZER(struct cmd_set_qmap_result,
> - map_value, UINT8);
> + map_value, UINT16);
>
> cmdline_parse_inst_t cmd_set_qmap = {
> .f = cmd_set_qmap_parsed,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 17a6efe..dfe5627 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -161,7 +161,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 = "########################";
>
> @@ -3742,7 +3742,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;
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe6450c..4b26c5c 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1840,7 +1840,7 @@ fwd_stats_display(void)
> fwd_cycles += fs->core_cycles;
> }
> 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];
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index c7e7e41..9ad002c 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -279,7 +279,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[];
> @@ -794,7 +794,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);
>
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index 4bcf220..1ef6f0e 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -178,6 +178,11 @@ API Changes
>
> * bpf: ``RTE_BPF_XTYPE_NUM`` has been dropped from ``rte_bpf_xtype``.
>
> +* ethdev: Data type of input parameter ``stat_idx`` in ``set_queue_stats_mapping``,
> + ``rte_eth_dev_set_tx_queue_stats_mapping`` and ``rte_eth_dev_set_rx_queue_stats_mapping``
> + function will be changed from ``uint8_t`` to ``uint16_t``, which supports for
> + using 256 or more than 256 queues and displaying all statistics of rx/tx queue.
> +
>
> ABI Changes
> -----------
> diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c
> index 810568b..875874e 100644
> --- a/drivers/net/igc/igc_ethdev.c
> +++ b/drivers/net/igc/igc_ethdev.c
> @@ -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
> @@ -2075,7 +2075,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);
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 8da963a..c6ea83a 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -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);
> @@ -890,7 +890,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
> diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
> index a19c0f3..29cea93 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -1356,7 +1356,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;
> diff --git a/drivers/net/octeontx2/otx2_ethdev.h b/drivers/net/octeontx2/otx2_ethdev.h
> index a114112..0c761e3 100644
> --- a/drivers/net/octeontx2/otx2_ethdev.h
> +++ b/drivers/net/octeontx2/otx2_ethdev.h
> @@ -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);
> diff --git a/drivers/net/octeontx2/otx2_stats.c b/drivers/net/octeontx2/otx2_stats.c
> index 8aaf270..6efe122 100644
> --- a/drivers/net/octeontx2/otx2_stats.c
> +++ b/drivers/net/octeontx2/otx2_stats.c
> @@ -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);
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 013a290..9d26012 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -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);
> @@ -2648,7 +2648,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;
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index dfe5c1b..749b372 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2942,7 +2942,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;
> @@ -2969,7 +2969,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));
> @@ -2978,7 +2978,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));
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 645a186..ff3a616 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2679,7 +2679,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
> @@ -2700,7 +2700,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.
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 23cc1e0..c68d465 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -222,7 +222,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. */
>
>
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.
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.
Can you please review the patch, is there any objectio to proceed with it?
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.
> 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.
>
> 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.
> >
> > 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/")
> >
> > 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>
> > ---
> > V4 -> V5:
> > add release notes updated.
> >
> > ---
> > v3->v4:
> > add a change in cmd_setqmap_mapvalue.
> >
> > ---
> > v2->v3:
> > change 'uint8_t i' to 'uint16_t i' in nic_stats_display function.
> >
> > ---
> > app/proc-info/main.c | 2 +-
> > app/test-pmd/cmdline.c | 4 ++--
> > app/test-pmd/config.c | 4 ++--
> > app/test-pmd/testpmd.c | 2 +-
> > app/test-pmd/testpmd.h | 5 +++--
> > doc/guides/rel_notes/release_20_11.rst | 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_driver.h | 2 +-
> > 15 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> > index 64fb83b..26d9355 100644
> > --- a/app/proc-info/main.c
> > +++ b/app/proc-info/main.c
> > @@ -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 = "########################";
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > index 08e123f..23e624f 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -8315,7 +8315,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
> > @@ -8346,7 +8346,7 @@ cmdline_parse_token_num_t cmd_setqmap_queueid =
> > queue_id, UINT16);
> > cmdline_parse_token_num_t cmd_setqmap_mapvalue =
> > TOKEN_NUM_INITIALIZER(struct cmd_set_qmap_result,
> > - map_value, UINT8);
> > + map_value, UINT16);
> >
> > cmdline_parse_inst_t cmd_set_qmap = {
> > .f = cmd_set_qmap_parsed,
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> > index 17a6efe..dfe5627 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -161,7 +161,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 = "########################";
> >
> > @@ -3742,7 +3742,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;
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index fe6450c..4b26c5c 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -1840,7 +1840,7 @@ fwd_stats_display(void)
> > fwd_cycles += fs->core_cycles;
> > }
> > 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];
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > index c7e7e41..9ad002c 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -279,7 +279,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[];
> > @@ -794,7 +794,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);
> >
> > diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> > index 4bcf220..1ef6f0e 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -178,6 +178,11 @@ API Changes
> >
> > * bpf: ``RTE_BPF_XTYPE_NUM`` has been dropped from ``rte_bpf_xtype``.
> >
> > +* ethdev: Data type of input parameter ``stat_idx`` in ``set_queue_stats_mapping``,
> > + ``rte_eth_dev_set_tx_queue_stats_mapping`` and ``rte_eth_dev_set_rx_queue_stats_mapping``
> > + function will be changed from ``uint8_t`` to ``uint16_t``, which supports for
> > + using 256 or more than 256 queues and displaying all statistics of rx/tx queue.
> > +
> >
> > ABI Changes
> > -----------
> > diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c
> > index 810568b..875874e 100644
> > --- a/drivers/net/igc/igc_ethdev.c
> > +++ b/drivers/net/igc/igc_ethdev.c
> > @@ -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
> > @@ -2075,7 +2075,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);
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 8da963a..c6ea83a 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -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);
> > @@ -890,7 +890,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
> > diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
> > index a19c0f3..29cea93 100644
> > --- a/drivers/net/memif/rte_eth_memif.c
> > +++ b/drivers/net/memif/rte_eth_memif.c
> > @@ -1356,7 +1356,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;
> > diff --git a/drivers/net/octeontx2/otx2_ethdev.h b/drivers/net/octeontx2/otx2_ethdev.h
> > index a114112..0c761e3 100644
> > --- a/drivers/net/octeontx2/otx2_ethdev.h
> > +++ b/drivers/net/octeontx2/otx2_ethdev.h
> > @@ -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);
> > diff --git a/drivers/net/octeontx2/otx2_stats.c b/drivers/net/octeontx2/otx2_stats.c
> > index 8aaf270..6efe122 100644
> > --- a/drivers/net/octeontx2/otx2_stats.c
> > +++ b/drivers/net/octeontx2/otx2_stats.c
> > @@ -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);
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index 013a290..9d26012 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -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);
> > @@ -2648,7 +2648,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;
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index dfe5c1b..749b372 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -2942,7 +2942,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;
> > @@ -2969,7 +2969,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));
> > @@ -2978,7 +2978,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));
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index 645a186..ff3a616 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -2679,7 +2679,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
> > @@ -2700,7 +2700,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.
> > diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> > index 23cc1e0..c68d465 100644
> > --- a/lib/librte_ethdev/rte_ethdev_driver.h
> > +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> > @@ -222,7 +222,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. */
> >
> >
>
> 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.
>
> 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.
>
> Can you please review the patch, is there any objectio to proceed with it?
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 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.it
> 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.
It is probably a bit unusual specially for modern NICs,
but I think it still possible in principle.
Let say NIC has up to 16K queues, but can map stats only for 1K, or so.
So formally - yes, I think both queue_idx and stats_idx have to be same type.
Also I can't foresee problems such change could introduce (except formal API/ABI break).
So:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> The problem is probably somewhere else (in testpmd),
> that's why I am against this patch.
>
在 2020/9/28 17:16, Thomas Monjalon 写道:
> 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?
>
Hi, Thomas,
you may misunderstand it.
The goal of this patch is that resolving some errors or warnings when
compiling code with RTE_ETHDEV_QUEUE_STAT_CNTRS > 256. The reson is some
vlaue(uint8_t) will never catch over RTE_ETHDEV_QUEUE_STAT_CNTRS, as
uint8_t ranges from 0-255. e.g.,
static void
nic_stats_display(uint16_t port_id)
{
uint8_t i;
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
...
}
...
}
RTE_ETHDEV_QUEUE_STAT_CNTRS can be increased to more than 256, which is
reasonable in principle. Because the number of queue statistics is
controlled by it. And mapping queue to queue statistics counter is also
controlled by it. So these "*_queue_stats_mapping" API also trigger
errors or warning.
> [...]
>>> --- 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.
>
>> 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.
>
First, my modification for "stat_idx" data type is resonable and it
does not cause some problems. Both queue id and stat_idx have same
type, which is also reasoable in usage.
Then, "*_queue_stats_mapping" these API is not only invented for ixgbe.
other net drivers also implements the API, like enic, igc, octeontx2 etc.
> The problem is probably somewhere else (in testpmd),
> that's why I am against this patch.
>
>
> .
>
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.
28/09/2020 15: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.
It is not said this API is supposed to manage more than 256 queues mapping.
In general we should not need this API.
I think it is solving the wrong problem.
28/09/2020 15:47, Min Hu (Connor):
>
> 在 2020/9/28 17:16, Thomas Monjalon 写道:
> > 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?
> >
> Hi, Thomas,
> you may misunderstand it.
> The goal of this patch is that resolving some errors or warnings when
> compiling code with RTE_ETHDEV_QUEUE_STAT_CNTRS > 256. The reson is some
> vlaue(uint8_t) will never catch over RTE_ETHDEV_QUEUE_STAT_CNTRS, as
> uint8_t ranges from 0-255. e.g.,
> static void
> nic_stats_display(uint16_t port_id)
> {
> uint8_t i;
> for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
> ...
> }
> ...
> }
What you are describing is not a problem, it is the API:
queue stats mapping is to map few queues (less than 256).
> RTE_ETHDEV_QUEUE_STAT_CNTRS can be increased to more than 256, which is
> reasonable in principle. Because the number of queue statistics is
> controlled by it. And mapping queue to queue statistics counter is also
> controlled by it.
No, queue statistics ID should be the queue ID (1:1 mapping),
except for rare limited HW.
> So these "*_queue_stats_mapping" API also trigger
> errors or warning.
>
> > [...]
> >>> --- 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.
> >
> >> 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.
> >
> First, my modification for "stat_idx" data type is resonable and it
> does not cause some problems.
Why do you think it is reasonnable?
What is the use case?
> Both queue id and stat_idx have same
> type, which is also reasoable in usage.
> Then, "*_queue_stats_mapping" these API is not only invented for ixgbe.
> other net drivers also implements the API, like enic, igc, octeontx2 etc.
I don't think enic implements it,
and I don't know what is the intent in igc and octeontx2
(maybe just a copy/paste).
> > The problem is probably somewhere else (in testpmd),
> > that's why I am against this patch.
We should start from the beginning: why do you increase this value?
Or we can go directly to the real issue:
testpmd shows queues statistics only if a mapping is defined.
This testpmd behaviour is absolutely wrong and should be fixed.
On Mon, 28 Sep 2020 17:24:26 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:
> 28/09/2020 15: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.
>
> It is not said this API is supposed to manage more than 256 queues mapping.
> In general we should not need this API.
> I think it is solving the wrong problem.
The original API is a band aid for the limited number of statistics counters
in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
have per-queue statistics and make ixgbe say "not supported"
在 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.
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".
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".
>
>
> .
>
On 28/09/2020 09:59, Ferruh Yigit wrote:
> 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.
>>
>> 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/")
>>
>> 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>
>> ---
>> V4 -> V5:
>> add release notes updated.
>>
>> ---
>> v3->v4:
>> add a change in cmd_setqmap_mapvalue.
>>
>> ---
>> v2->v3:
>> change 'uint8_t i' to 'uint16_t i' in nic_stats_display function.
>>
>> ---
>> app/proc-info/main.c | 2 +-
>> app/test-pmd/cmdline.c | 4 ++--
>> app/test-pmd/config.c | 4 ++--
>> app/test-pmd/testpmd.c | 2 +-
>> app/test-pmd/testpmd.h | 5 +++--
>> doc/guides/rel_notes/release_20_11.rst | 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_driver.h | 2 +-
>> 15 files changed, 29 insertions(+), 23 deletions(-)
>>
[SNIP]
>
> 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.
>
> 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.
>
> Can you please review the patch, is there any objectio to proceed with it?
After reading the rest of the thread, I understand that Thomas has suggested depreciating this entire API and using xstats instead.
My 2c is that if changing this value requires an ABI breakage and rebuild, its probably the wrong API.
Ray K
On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
> On Mon, 28 Sep 2020 17:24:26 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
>
>> 28/09/2020 15: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.
>>
>> It is not said this API is supposed to manage more than 256 queues mapping.
>> In general we should not need this API.
>> I think it is solving the wrong problem.
>
>
> The original API is a band aid for the limited number of statistics counters
> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
> have per-queue statistics and make ixgbe say "not supported"
>
The current issue is not directly related to '*_queue_stats_mapping' APIs.
Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is used
to define size of the stats counter.
"uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"
When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors, the
one in the ethdev is like [1].
This can be fixed two ways,
a) increase the size of 'stat_idx' storage type to u16 in the
'*_queue_stats_mapping' APIs, this is what this patch does.
b) Fix with a casting in the comparison, without changing the APIs.
I think both are OK, but is (b) more preferable?
[1]
../lib/librte_ethdev/rte_ethdev.c: In function ‘set_queue_stats_mapping’:
../lib/librte_ethdev/rte_ethdev.c:2943:15: warning: comparison is always false
due to limited range of data type [-Wtype-limits]
2943 | if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
| ^~
Hi,
On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote:
> On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
> > On Mon, 28 Sep 2020 17:24:26 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > > 28/09/2020 15: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.
> > >
> > > It is not said this API is supposed to manage more than 256 queues mapping.
> > > In general we should not need this API.
> > > I think it is solving the wrong problem.
> >
> >
> > The original API is a band aid for the limited number of statistics counters
> > in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
> > have per-queue statistics and make ixgbe say "not supported"
> >
>
> The current issue is not directly related to '*_queue_stats_mapping' APIs.
>
> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is
> used to define size of the stats counter.
> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"
>
> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors,
> the one in the ethdev is like [1].
>
> This can be fixed two ways,
> a) increase the size of 'stat_idx' storage type to u16 in the
> '*_queue_stats_mapping' APIs, this is what this patch does.
> b) Fix with a casting in the comparison, without changing the APIs.
>
> I think both are OK, but is (b) more preferable?
I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is
not modified.
On the substance, I agree with Thomas that the queue_stats_mapping API
should be replaced by xstats.
>
>
> [1]
> ../lib/librte_ethdev/rte_ethdev.c: In function ‘set_queue_stats_mapping’:
> ../lib/librte_ethdev/rte_ethdev.c:2943:15: warning: comparison is always
> false due to limited range of data type [-Wtype-limits]
> 2943 | if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
> | ^~
On 10/6/2020 9:33 AM, Olivier Matz wrote:
> Hi,
>
> On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote:
>> On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
>>> On Mon, 28 Sep 2020 17:24:26 +0200
>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>
>>>> 28/09/2020 15: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.
>>>>
>>>> It is not said this API is supposed to manage more than 256 queues mapping.
>>>> In general we should not need this API.
>>>> I think it is solving the wrong problem.
>>>
>>>
>>> The original API is a band aid for the limited number of statistics counters
>>> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
>>> have per-queue statistics and make ixgbe say "not supported"
>>>
>>
>> The current issue is not directly related to '*_queue_stats_mapping' APIs.
>>
>> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
>> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is
>> used to define size of the stats counter.
>> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"
>>
>> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors,
>> the one in the ethdev is like [1].
>>
>> This can be fixed two ways,
>> a) increase the size of 'stat_idx' storage type to u16 in the
>> '*_queue_stats_mapping' APIs, this is what this patch does.
>> b) Fix with a casting in the comparison, without changing the APIs.
>>
>> I think both are OK, but is (b) more preferable?
>
> I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is
> not modified.
>
> On the substance, I agree with Thomas that the queue_stats_mapping API
> should be replaced by xstats.
>
This has been discussed in the last technical board meeting, the decision was to
use xstats to get queue related statistics [2].
But after second look, even if xstats is used to get statistics,
'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses 'rte_eth_stats_get()'
to get queue statistics.
So for the case device has more than 255 queues, 'RTE_ETHDEV_QUEUE_STAT_CNTRS'
still needs to be set > 255 which will cause the build error.
I have an AR to send a deprecation notice to current method to get the queue
statistics, and limit the old method to 256 queues. But since xstats is just a
wrapped to old method, I am not quite sure how deprecating it will work.
@Thomas, @Honnappa, can you give some more insight on the issue?
[2]
https://mails.dpdk.org/archives/dev/2020-October/185299.html
>
>>
>>
>> [1]
>> ../lib/librte_ethdev/rte_ethdev.c: In function ‘set_queue_stats_mapping’:
>> ../lib/librte_ethdev/rte_ethdev.c:2943:15: warning: comparison is always
>> false due to limited range of data type [-Wtype-limits]
>> 2943 | if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
>> | ^~
09/10/2020 22:32, Ferruh Yigit:
> On 10/6/2020 9:33 AM, Olivier Matz wrote:
> > On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote:
> >> On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
> >>> On Mon, 28 Sep 2020 17:24:26 +0200
> >>> Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>> 28/09/2020 15: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.
> >>>>
> >>>> It is not said this API is supposed to manage more than 256 queues mapping.
> >>>> In general we should not need this API.
> >>>> I think it is solving the wrong problem.
> >>>
> >>>
> >>> The original API is a band aid for the limited number of statistics counters
> >>> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
> >>> have per-queue statistics and make ixgbe say "not supported"
> >>>
> >>
> >> The current issue is not directly related to '*_queue_stats_mapping' APIs.
> >>
> >> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
> >> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is
> >> used to define size of the stats counter.
> >> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"
> >>
> >> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors,
> >> the one in the ethdev is like [1].
> >>
> >> This can be fixed two ways,
> >> a) increase the size of 'stat_idx' storage type to u16 in the
> >> '*_queue_stats_mapping' APIs, this is what this patch does.
> >> b) Fix with a casting in the comparison, without changing the APIs.
> >>
> >> I think both are OK, but is (b) more preferable?
> >
> > I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is
> > not modified.
> >
> > On the substance, I agree with Thomas that the queue_stats_mapping API
> > should be replaced by xstats.
> >
>
> This has been discussed in the last technical board meeting, the decision was to
> use xstats to get queue related statistics [2].
>
> But after second look, even if xstats is used to get statistics,
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses 'rte_eth_stats_get()'
> to get queue statistics.
> So for the case device has more than 255 queues, 'RTE_ETHDEV_QUEUE_STAT_CNTRS'
> still needs to be set > 255 which will cause the build error.
You're right, when using the old API in xstats implementation,
we are limited to RTE_ETHDEV_QUEUE_STAT_CNTRS queues.
> I have an AR to send a deprecation notice to current method to get the queue
> statistics, and limit the old method to 256 queues. But since xstats is just a
> wrapped to old method, I am not quite sure how deprecating it will work.
>
> @Thomas, @Honnappa, can you give some more insight on the issue?
It becomes a PMD issue. The PMD implementation of xstats must complete
the statistics for the queues above RTE_ETHDEV_QUEUE_STAT_CNTRS.
In order to prepare the removal of the old method smoothly,
we could add a driver flag which indicates whether the PMD relies
on a pre-fill of xstats from old stats per queue conversion or not.
> [2]
> https://mails.dpdk.org/archives/dev/2020-October/185299.html
On 10/10/2020 9:09 AM, Thomas Monjalon wrote:
> 09/10/2020 22:32, Ferruh Yigit:
>> On 10/6/2020 9:33 AM, Olivier Matz wrote:
>>> On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote:
>>>> On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
>>>>> On Mon, 28 Sep 2020 17:24:26 +0200
>>>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>> 28/09/2020 15: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.
>>>>>>
>>>>>> It is not said this API is supposed to manage more than 256 queues mapping.
>>>>>> In general we should not need this API.
>>>>>> I think it is solving the wrong problem.
>>>>>
>>>>>
>>>>> The original API is a band aid for the limited number of statistics counters
>>>>> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
>>>>> have per-queue statistics and make ixgbe say "not supported"
>>>>>
>>>>
>>>> The current issue is not directly related to '*_queue_stats_mapping' APIs.
>>>>
>>>> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
>>>> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is
>>>> used to define size of the stats counter.
>>>> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"
>>>>
>>>> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors,
>>>> the one in the ethdev is like [1].
>>>>
>>>> This can be fixed two ways,
>>>> a) increase the size of 'stat_idx' storage type to u16 in the
>>>> '*_queue_stats_mapping' APIs, this is what this patch does.
>>>> b) Fix with a casting in the comparison, without changing the APIs.
>>>>
>>>> I think both are OK, but is (b) more preferable?
>>>
>>> I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is
>>> not modified.
>>>
>>> On the substance, I agree with Thomas that the queue_stats_mapping API
>>> should be replaced by xstats.
>>>
>>
>> This has been discussed in the last technical board meeting, the decision was to
>> use xstats to get queue related statistics [2].
>>
>> But after second look, even if xstats is used to get statistics,
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses 'rte_eth_stats_get()'
>> to get queue statistics.
>> So for the case device has more than 255 queues, 'RTE_ETHDEV_QUEUE_STAT_CNTRS'
>> still needs to be set > 255 which will cause the build error.
>
> You're right, when using the old API in xstats implementation,
> we are limited to RTE_ETHDEV_QUEUE_STAT_CNTRS queues.
>
>> I have an AR to send a deprecation notice to current method to get the queue
>> statistics, and limit the old method to 256 queues. But since xstats is just a
>> wrapped to old method, I am not quite sure how deprecating it will work.
>>
>> @Thomas, @Honnappa, can you give some more insight on the issue?
>
> It becomes a PMD issue. The PMD implementation of xstats must complete
> the statistics for the queues above RTE_ETHDEV_QUEUE_STAT_CNTRS.
>
> In order to prepare the removal of the old method smoothly,
> we could add a driver flag which indicates whether the PMD relies
> on a pre-fill of xstats from old stats per queue conversion or not.
>
I have sent an RFC, can you please check:
https://patches.dpdk.org/patch/80390/
Connor,
Does this proposal make sense?
If you have more than 256 queues to get stats, can you please implement xstats
for the queue stats?
You don't need to wait for the above RFC accepted, you can implement the xstats,
but it will have some duplication, if the above RFC accepted you can set the
'RTE_ETH_DEV_QUEUE_STATS_IN_XSTATS' flag to remove the duplication.
>
>> [2]
>> https://mails.dpdk.org/archives/dev/2020-October/185299.html
>
>
>
@@ -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 = "########################";
@@ -8315,7 +8315,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
@@ -8346,7 +8346,7 @@ cmdline_parse_token_num_t cmd_setqmap_queueid =
queue_id, UINT16);
cmdline_parse_token_num_t cmd_setqmap_mapvalue =
TOKEN_NUM_INITIALIZER(struct cmd_set_qmap_result,
- map_value, UINT8);
+ map_value, UINT16);
cmdline_parse_inst_t cmd_set_qmap = {
.f = cmd_set_qmap_parsed,
@@ -161,7 +161,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 = "########################";
@@ -3742,7 +3742,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;
@@ -1840,7 +1840,7 @@ fwd_stats_display(void)
fwd_cycles += fs->core_cycles;
}
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];
@@ -279,7 +279,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[];
@@ -794,7 +794,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);
@@ -178,6 +178,11 @@ API Changes
* bpf: ``RTE_BPF_XTYPE_NUM`` has been dropped from ``rte_bpf_xtype``.
+* ethdev: Data type of input parameter ``stat_idx`` in ``set_queue_stats_mapping``,
+ ``rte_eth_dev_set_tx_queue_stats_mapping`` and ``rte_eth_dev_set_rx_queue_stats_mapping``
+ function will be changed from ``uint8_t`` to ``uint16_t``, which supports for
+ using 256 or more than 256 queues and displaying all statistics of rx/tx queue.
+
ABI Changes
-----------
@@ -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
@@ -2075,7 +2075,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);
@@ -890,7 +890,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
@@ -1356,7 +1356,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);
@@ -2648,7 +2648,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;
@@ -2942,7 +2942,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;
@@ -2969,7 +2969,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));
@@ -2978,7 +2978,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));
@@ -2679,7 +2679,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
@@ -2700,7 +2700,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.
@@ -222,7 +222,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. */