[dpdk-dev,dpdk-dev,PATCHv6,1/6] ethdev: enhance rte_eth_(tx|rx)q_info struct

  Add 2 fields in struct rte_eth_(tx|rx)q_info :
- used_desc : for used queue descriptors
- free_desc : for free queue descriptors
for ability to query more information from queues.

Signed-off-by: Amine Kherbouche <amine.kherbouche@6wind.com>
---
 lib/librte_ether/rte_ethdev.h |    4 ++++
 1 file changed, 4 insertions(+)
  

On Tue, 20 Oct 2015 00:06:19 +0200
Amine Kherbouche <amine.kherbouche@6wind.com> wrote:

> +	uint16_t used_desc;         /**< number of used descriptors */
> +	uint16_t free_desc;         /**< number of free descriptors */

These two fields are obviously not SMP sfe.
Also, they are redundant with the existing queue_count API?
  

Hi Amine,

> -----Original Message-----
> From: Amine Kherbouche [mailto:amine.kherbouche@6wind.com]
> Sent: Monday, October 19, 2015 11:06 PM
> To: dev@dpdk.org
> Cc: amine.kherbouche@6wind.com; vincent.jardin@6wind.com; Ananyev, Konstantin
> Subject: [dpdk-dev,PATCHv6 1/6] ethdev: enhance rte_eth_(tx|rx)q_info struct
> 
> Add 2 fields in struct rte_eth_(tx|rx)q_info :
> - used_desc : for used queue descriptors
> - free_desc : for free queue descriptors
> for ability to query more information from queues.

As I can see your patch series should be applied on top of mine:
[PATCHv5 0/8] ethdev: add new API to retrieve RX/TX queue information
Which is not yet in the dpdk.org mainline.
I believe that is ok, but then you shouldn't replace previous version with the patch (v5)
with new one, but create a new patch for your changes and clearly state that dependency.

Konstantin

> 
> Signed-off-by: Amine Kherbouche <amine.kherbouche@6wind.com>
> ---
>  lib/librte_ether/rte_ethdev.h |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 4d7b6f2..5fc86a0 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -874,6 +874,8 @@ struct rte_eth_rxq_info {
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> +	uint16_t used_desc;         /**< number of used descriptors */
> +	uint16_t free_desc;         /**< number of free descriptors */
>  } __rte_cache_aligned;
> 
>  /**
> @@ -883,6 +885,8 @@ struct rte_eth_rxq_info {
>  struct rte_eth_txq_info {
>  	struct rte_eth_txconf conf; /**< queue config parameters. */
>  	uint16_t nb_desc;           /**< configured number of TXDs. */
> +	uint16_t used_desc;         /**< number of used descriptors */
> +	uint16_t free_desc;         /**< number of free descriptors */
>  } __rte_cache_aligned;
> 
>  struct rte_eth_dev;
> --
> 1.7.10.4
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Monday, October 19, 2015 11:44 PM
> To: Amine Kherbouche
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-dev, PATCHv6 1/6] ethdev: enhance rte_eth_(tx|rx)q_info struct
> 
> On Tue, 20 Oct 2015 00:06:19 +0200
> Amine Kherbouche <amine.kherbouche@6wind.com> wrote:
> 
> > +	uint16_t used_desc;         /**< number of used descriptors */
> > +	uint16_t free_desc;         /**< number of free descriptors */
> 
> These two fields are obviously not SMP sfe.

I don't think they have to be.
From my thinking it is just a snapshot of SW state of the queue, that could be used for statistics/watchdog/debugging purposes.

> Also, they are redundant with the existing queue_count API?

Yep, similar thought here:
In the for Intel HW implementations:
qinfo->used_desc = ixgbe_dev_rx_queue_count(dev, queue_id);
It seems a bit redundant, as if user wants to know HW state it can call rte_eth_rx_queue_count() directly.
From other side:  rte_eth_rx_queue_count() is quite heavyweight function, as it scans HW descriptors to check their status.
I think it should be better to return here just a snapshot of SW state: how many free/used RXDs are from PMD perspective
(by collecting info from *_rx_queue structure, without reading actual HW registers/descriptors - as you done for TX queue info).

Konstantin
  

> Yep, similar thought here:
> In the for Intel HW implementations:
> qinfo->used_desc = ixgbe_dev_rx_queue_count(dev, queue_id);
> It seems a bit redundant, as if user wants to know HW state it can call
> rte_eth_rx_queue_count() directly.
> From other side:  rte_eth_rx_queue_count() is quite heavyweight function,
> as it scans HW descriptors to check their status.
> I think it should be better to return here just a snapshot of SW state:
> how many free/used RXDs are from PMD perspective
> (by collecting info from *_rx_queue structure, without reading actual HW
> registers/descriptors - as you done for TX queue info).
>
> Konstantin
>

Yes,

Konstantin is right, it is a light solution to get an snapshot of the state
of the queues.
  

2015-10-20 09:36, Ananyev, Konstantin:
> As I can see your patch series should be applied on top of mine:
> [PATCHv5 0/8] ethdev: add new API to retrieve RX/TX queue information
> Which is not yet in the dpdk.org mainline.
> I believe that is ok, but then you shouldn't replace previous version with the patch (v5)
> with new one, but create a new patch for your changes and clearly state that dependency.

Yes, Amine, this is not a new version of Konstantin's patches.
So you must not use v6 and you should mention in the cover letter that it
depends on Konstantin's patches.
Please use v2 next time.