[dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: The hairpin queue is not supported with above
rte_eth_*x_queue_info_get, so the queue state could be
RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 doc/guides/rel_notes/release_21_05.rst | 6 ++++++
 lib/librte_ethdev/rte_ethdev.c         | 3 +++
 lib/librte_ethdev/rte_ethdev.h         | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 43063e3..165b5f7 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -156,6 +156,12 @@ ABI Changes
 
 * No ABI change that would break compatibility with 20.11.
 
+* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
+  to provide indicated rxq queue state.
+
+* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
+  to provide indicated txq queue state.
+
 
 Known Issues
 ------------
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 3059aa5..fbd10b2 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index efda313..3b83c5a 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
+	/**< Queues state: STARTED(1) / STOPPED(0). */
+	uint8_t queue_state;
 } __rte_cache_min_aligned;
 
 /**
@@ -1600,6 +1602,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. */
+	/**< Queues state: STARTED(1) / STOPPED(0). */
+	uint8_t queue_state;
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 3/18/2021 12:25 PM, Lijun Ou wrote:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: The hairpin queue is not supported with above
> rte_eth_*x_queue_info_get, so the queue state could be
> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

<...>

> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index efda313..3b83c5a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>   	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>   	uint16_t nb_desc;           /**< configured number of RXDs. */
>   	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */
> +	uint8_t queue_state;
>   } __rte_cache_min_aligned;
>   
>   /**
> @@ -1600,6 +1602,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. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */
> +	uint8_t queue_state;
>   } __rte_cache_min_aligned;
>   
>   /* Generic Burst mode flag definition, values can be ORed. */
> 

This is causing an ABI warning [1], but I guess it is safe since the size of the 
struct is not changing (cache align). Adding a few more people to comment.


[1]
https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Kinsella, Ray]

On 22/03/2021 09:22, Ferruh Yigit wrote:
> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: The hairpin queue is not supported with above
>> rte_eth_*x_queue_info_get, so the queue state could be
>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> 
> <...>
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index efda313..3b83c5a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>       uint16_t nb_desc;           /**< configured number of RXDs. */
>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>     /**
>> @@ -1600,6 +1602,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. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>     /* Generic Burst mode flag definition, values can be ORed. */
>>
> 
> This is causing an ABI warning [1], but I guess it is safe since the size of the struct is not changing (cache align). Adding a few more people to comment.

Agreed, needs an amendment to libabigail.ignore to condone. 

> 
> [1]
> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/3/22 17:22, Ferruh Yigit 写道:
> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: The hairpin queue is not supported with above
>> rte_eth_*x_queue_info_get, so the queue state could be
>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> 
> <...>
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.h 
>> b/lib/librte_ethdev/rte_ethdev.h
>> index efda313..3b83c5a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>       uint16_t nb_desc;           /**< configured number of RXDs. */
>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>   /**
>> @@ -1600,6 +1602,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. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>   /* Generic Burst mode flag definition, values can be ORed. */
>>
> 
> This is causing an ABI warning [1], but I guess it is safe since the 
> size of the struct is not changing (cache align). Adding a few more 
> people to comment.
> 
Yes, thanks.Because we've analyzed it internally.What about other 
people's opinions?
> 
> [1]
> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> .
> 

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Andrew Rybchenko]

On 3/22/21 12:22 PM, Ferruh Yigit wrote:
> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: The hairpin queue is not supported with above
>> rte_eth_*x_queue_info_get, so the queue state could be
>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> 
> <...>
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>> b/lib/librte_ethdev/rte_ethdev.h
>> index efda313..3b83c5a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>       uint16_t nb_desc;           /**< configured number of RXDs. */
>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>     /**
>> @@ -1600,6 +1602,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. */
>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>> +    uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>     /* Generic Burst mode flag definition, values can be ORed. */
>>
> 
> This is causing an ABI warning [1], but I guess it is safe since the
> size of the struct is not changing (cache align). Adding a few more
> people to comment.
> 
> 
> [1]
> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651

Frankly speaking I dislike addition of queue_state as uint8_t.
IMHO it should be either 'bool started' or enum to support more
states in the future if we need.

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> Sent: Monday, March 22, 2021 2:49 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> 
> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
> > On 3/18/2021 12:25 PM, Lijun Ou wrote:
> >> Currently, upper-layer application could get queue state only
> >> through pointers such as dev->data->tx_queue_state[queue_id],
> >> this is not the recommended way to access it. So this patch
> >> add get queue state when call rte_eth_rx_queue_info_get and
> >> rte_eth_tx_queue_info_get API.
> >>
> >> Note: The hairpin queue is not supported with above
> >> rte_eth_*x_queue_info_get, so the queue state could be
> >> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >> it could be ABI compatible.
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >
> > <...>
> >
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >> b/lib/librte_ethdev/rte_ethdev.h
> >> index efda313..3b83c5a 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
> >>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>       uint16_t nb_desc;           /**< configured number of RXDs. */
> >>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >> +    uint8_t queue_state;
> >>   } __rte_cache_min_aligned;
> >>     /**
> >> @@ -1600,6 +1602,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. */
> >> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >> +    uint8_t queue_state;
> >>   } __rte_cache_min_aligned;
> >>     /* Generic Burst mode flag definition, values can be ORed. */
> >>
> >
> > This is causing an ABI warning [1], but I guess it is safe since the
> > size of the struct is not changing (cache align). Adding a few more
> > people to comment.
> >
> >
> > [1]
> > https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> 
> Frankly speaking I dislike addition of queue_state as uint8_t.
> IMHO it should be either 'bool started' or enum to support more
> states in the future if we need.

I think we already have set of defines for it:
lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2

If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.

About uint8_t vs enum - yes, in principle enum would be a bit nicer,
but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
Unless in future will want to change it in struct rte_eth_dev_data too
(or even hide it inside dev private queue data). 
  

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Andrew Rybchenko]

On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>> Sent: Monday, March 22, 2021 2:49 PM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>
>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>>>> Currently, upper-layer application could get queue state only
>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>> this is not the recommended way to access it. So this patch
>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>> rte_eth_tx_queue_info_get API.
>>>>
>>>> Note: The hairpin queue is not supported with above
>>>> rte_eth_*x_queue_info_get, so the queue state could be
>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>> it could be ABI compatible.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>
>>> <...>
>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>> index efda313..3b83c5a 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>       uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>> +    uint8_t queue_state;
>>>>   } __rte_cache_min_aligned;
>>>>     /**
>>>> @@ -1600,6 +1602,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. */
>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>> +    uint8_t queue_state;
>>>>   } __rte_cache_min_aligned;
>>>>     /* Generic Burst mode flag definition, values can be ORed. */
>>>>
>>>
>>> This is causing an ABI warning [1], but I guess it is safe since the
>>> size of the struct is not changing (cache align). Adding a few more
>>> people to comment.
>>>
>>>
>>> [1]
>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
>>
>> Frankly speaking I dislike addition of queue_state as uint8_t.
>> IMHO it should be either 'bool started' or enum to support more
>> states in the future if we need.
> 
> I think we already have set of defines for it:
> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> 
> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> 
> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> Unless in future will want to change it in struct rte_eth_dev_data too
> (or even hide it inside dev private queue data). 

I forgot about hairpin and bitmask... If so, I think it is
sufficient to fix absolutely misleading comment, say
that it is a bit mask and think about removal of
RTE_ETH_QUEUE_STATE_STOPPED (since it could be
stopped+hairpin). May be consider to use uin16_t,
since 8 bit is really small bitmask. It still fits in
available hole.

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, March 22, 2021 4:02 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> 
> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> >> Sent: Monday, March 22, 2021 2:49 PM
> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> >> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> >> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> >>
> >> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
> >>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
> >>>> Currently, upper-layer application could get queue state only
> >>>> through pointers such as dev->data->tx_queue_state[queue_id],
> >>>> this is not the recommended way to access it. So this patch
> >>>> add get queue state when call rte_eth_rx_queue_info_get and
> >>>> rte_eth_tx_queue_info_get API.
> >>>>
> >>>> Note: The hairpin queue is not supported with above
> >>>> rte_eth_*x_queue_info_get, so the queue state could be
> >>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>>> it could be ABI compatible.
> >>>>
> >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>
> >>> <...>
> >>>
> >>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>> b/lib/librte_ethdev/rte_ethdev.h
> >>>> index efda313..3b83c5a 100644
> >>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
> >>>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>>>       uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>> +    uint8_t queue_state;
> >>>>   } __rte_cache_min_aligned;
> >>>>     /**
> >>>> @@ -1600,6 +1602,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. */
> >>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>> +    uint8_t queue_state;
> >>>>   } __rte_cache_min_aligned;
> >>>>     /* Generic Burst mode flag definition, values can be ORed. */
> >>>>
> >>>
> >>> This is causing an ABI warning [1], but I guess it is safe since the
> >>> size of the struct is not changing (cache align). Adding a few more
> >>> people to comment.
> >>>
> >>>
> >>> [1]
> >>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> >>
> >> Frankly speaking I dislike addition of queue_state as uint8_t.
> >> IMHO it should be either 'bool started' or enum to support more
> >> states in the future if we need.
> >
> > I think we already have set of defines for it:
> > lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> > lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> > lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> >
> > If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> >
> > About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> > but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> > So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> > Unless in future will want to change it in struct rte_eth_dev_data too
> > (or even hide it inside dev private queue data).
> 
> I forgot about hairpin and bitmask... If so, I think it is
> sufficient to fix absolutely misleading comment, say
> that it is a bit mask and think about removal of
> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
> stopped+hairpin). May be consider to use uin16_t,
> since 8 bit is really small bitmask. It still fits in
> available hole.

Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
and each of the states (stopped/started/hairpin) is mutually exclusive.
Is that not what was intended when hairpin queues were introduced?

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Andrew Rybchenko]

On 3/22/21 7:53 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, March 22, 2021 4:02 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
>> thomas@monjalon.net
>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>
>> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>>>> Sent: Monday, March 22, 2021 2:49 PM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>>>
>>>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
>>>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>>>>>> Currently, upper-layer application could get queue state only
>>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>>>> this is not the recommended way to access it. So this patch
>>>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>>>> rte_eth_tx_queue_info_get API.
>>>>>>
>>>>>> Note: The hairpin queue is not supported with above
>>>>>> rte_eth_*x_queue_info_get, so the queue state could be
>>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>>>> it could be ABI compatible.
>>>>>>
>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>
>>>>> <...>
>>>>>
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>>>> index efda313..3b83c5a 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>>>>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>>>       uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>> +    uint8_t queue_state;
>>>>>>   } __rte_cache_min_aligned;
>>>>>>     /**
>>>>>> @@ -1600,6 +1602,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. */
>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>> +    uint8_t queue_state;
>>>>>>   } __rte_cache_min_aligned;
>>>>>>     /* Generic Burst mode flag definition, values can be ORed. */
>>>>>>
>>>>>
>>>>> This is causing an ABI warning [1], but I guess it is safe since the
>>>>> size of the struct is not changing (cache align). Adding a few more
>>>>> people to comment.
>>>>>
>>>>>
>>>>> [1]
>>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
>>>>
>>>> Frankly speaking I dislike addition of queue_state as uint8_t.
>>>> IMHO it should be either 'bool started' or enum to support more
>>>> states in the future if we need.
>>>
>>> I think we already have set of defines for it:
>>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
>>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
>>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>>>
>>> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
>>>
>>> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
>>> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
>>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
>>> Unless in future will want to change it in struct rte_eth_dev_data too
>>> (or even hide it inside dev private queue data).
>>
>> I forgot about hairpin and bitmask... If so, I think it is
>> sufficient to fix absolutely misleading comment, say
>> that it is a bit mask and think about removal of
>> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
>> stopped+hairpin). May be consider to use uin16_t,
>> since 8 bit is really small bitmask. It still fits in
>> available hole.
> 
> Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
> and each of the states (stopped/started/hairpin) is mutually exclusive.
> Is that not what was intended when hairpin queues were introduced?
> 

Thanks, yes, you're right. My memory lies to me. If queue state
is not a bit mask, it should be an enum from API point of view.
Rx/Tx queue info structures are control path. I see no point to
save bits here. Clear API is more important on control path.
The only reason here to use uint8_t is to avoid ABI breakage.
I can't judge if it is critical to wait or not.

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, March 22, 2021 5:08 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> 
> On 3/22/21 7:53 PM, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, March 22, 2021 4:02 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
> >> thomas@monjalon.net
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> >> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> >> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> >>
> >> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> >>>> Sent: Monday, March 22, 2021 2:49 PM
> >>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
> >>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> >>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> >>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> >>>>
> >>>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
> >>>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
> >>>>>> Currently, upper-layer application could get queue state only
> >>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
> >>>>>> this is not the recommended way to access it. So this patch
> >>>>>> add get queue state when call rte_eth_rx_queue_info_get and
> >>>>>> rte_eth_tx_queue_info_get API.
> >>>>>>
> >>>>>> Note: The hairpin queue is not supported with above
> >>>>>> rte_eth_*x_queue_info_get, so the queue state could be
> >>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>>>>> it could be ABI compatible.
> >>>>>>
> >>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>>>> b/lib/librte_ethdev/rte_ethdev.h
> >>>>>> index efda313..3b83c5a 100644
> >>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
> >>>>>>       uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>>>>>       uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>>>>       uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>>>> +    uint8_t queue_state;
> >>>>>>   } __rte_cache_min_aligned;
> >>>>>>     /**
> >>>>>> @@ -1600,6 +1602,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. */
> >>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>>>> +    uint8_t queue_state;
> >>>>>>   } __rte_cache_min_aligned;
> >>>>>>     /* Generic Burst mode flag definition, values can be ORed. */
> >>>>>>
> >>>>>
> >>>>> This is causing an ABI warning [1], but I guess it is safe since the
> >>>>> size of the struct is not changing (cache align). Adding a few more
> >>>>> people to comment.
> >>>>>
> >>>>>
> >>>>> [1]
> >>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> >>>>
> >>>> Frankly speaking I dislike addition of queue_state as uint8_t.
> >>>> IMHO it should be either 'bool started' or enum to support more
> >>>> states in the future if we need.
> >>>
> >>> I think we already have set of defines for it:
> >>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> >>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> >>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> >>>
> >>> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> >>>
> >>> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> >>> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> >>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> >>> Unless in future will want to change it in struct rte_eth_dev_data too
> >>> (or even hide it inside dev private queue data).
> >>
> >> I forgot about hairpin and bitmask... If so, I think it is
> >> sufficient to fix absolutely misleading comment, say
> >> that it is a bit mask and think about removal of
> >> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
> >> stopped+hairpin). May be consider to use uin16_t,
> >> since 8 bit is really small bitmask. It still fits in
> >> available hole.
> >
> > Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
> > and each of the states (stopped/started/hairpin) is mutually exclusive.
> > Is that not what was intended when hairpin queues were introduced?
> >
> 
> Thanks, yes, you're right. My memory lies to me. If queue state
> is not a bit mask, it should be an enum from API point of view.
> Rx/Tx queue info structures are control path. I see no point to
> save bits here. Clear API is more important on control path.
> The only reason here to use uint8_t is to avoid ABI breakage.
> I can't judge if it is critical to wait or not.

As alternate thought - introduce new API function,
something like:
int rte_eth_get_rxq_state(portid, queue_id);
Then rte_eth_dev_is_rx_hairpin_queue() probably can be deprecated
in favour of this new one.  

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 3/22/2021 6:53 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, March 22, 2021 5:08 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
>> thomas@monjalon.net
>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>
>> On 3/22/21 7:53 PM, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Monday, March 22, 2021 4:02 PM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
>>>> thomas@monjalon.net
>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>>>
>>>> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>>>>>> Sent: Monday, March 22, 2021 2:49 PM
>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
>>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>>>>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>>>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>>>>>
>>>>>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
>>>>>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>>>>>>>> Currently, upper-layer application could get queue state only
>>>>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>>>>>> this is not the recommended way to access it. So this patch
>>>>>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>>>>>> rte_eth_tx_queue_info_get API.
>>>>>>>>
>>>>>>>> Note: The hairpin queue is not supported with above
>>>>>>>> rte_eth_*x_queue_info_get, so the queue state could be
>>>>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>>>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>>>>>> it could be ABI compatible.
>>>>>>>>
>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>
>>>>>>> <...>
>>>>>>>
>>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> index efda313..3b83c5a 100644
>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>>>>>>>        uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>>>>>        uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>>>>>        uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>>>> +    uint8_t queue_state;
>>>>>>>>    } __rte_cache_min_aligned;
>>>>>>>>      /**
>>>>>>>> @@ -1600,6 +1602,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. */
>>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>>>> +    uint8_t queue_state;
>>>>>>>>    } __rte_cache_min_aligned;
>>>>>>>>      /* Generic Burst mode flag definition, values can be ORed. */
>>>>>>>>
>>>>>>>
>>>>>>> This is causing an ABI warning [1], but I guess it is safe since the
>>>>>>> size of the struct is not changing (cache align). Adding a few more
>>>>>>> people to comment.
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
>>>>>>
>>>>>> Frankly speaking I dislike addition of queue_state as uint8_t.
>>>>>> IMHO it should be either 'bool started' or enum to support more
>>>>>> states in the future if we need.
>>>>>
>>>>> I think we already have set of defines for it:
>>>>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
>>>>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
>>>>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>>>>>
>>>>> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
>>>>>
>>>>> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
>>>>> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
>>>>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
>>>>> Unless in future will want to change it in struct rte_eth_dev_data too
>>>>> (or even hide it inside dev private queue data).
>>>>
>>>> I forgot about hairpin and bitmask... If so, I think it is
>>>> sufficient to fix absolutely misleading comment, say
>>>> that it is a bit mask and think about removal of
>>>> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
>>>> stopped+hairpin). May be consider to use uin16_t,
>>>> since 8 bit is really small bitmask. It still fits in
>>>> available hole.
>>>
>>> Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
>>> and each of the states (stopped/started/hairpin) is mutually exclusive.
>>> Is that not what was intended when hairpin queues were introduced?
>>>
>>
>> Thanks, yes, you're right. My memory lies to me. If queue state
>> is not a bit mask, it should be an enum from API point of view.
>> Rx/Tx queue info structures are control path. I see no point to
>> save bits here. Clear API is more important on control path.
>> The only reason here to use uint8_t is to avoid ABI breakage.
>> I can't judge if it is critical to wait or not.
> 
> As alternate thought - introduce new API function,
> something like:
> int rte_eth_get_rxq_state(portid, queue_id);
> Then rte_eth_dev_is_rx_hairpin_queue() probably can be deprecated
> in favour of this new one.
> 
> 

The 'rte_eth_dev_is_rx_hairpin_queue()' is internal function, and it is not 
visible to the application, it should be OK to keep it.

But 'STATE_HAIRPIN' should be kept internal, or should be available to the 
application?

The actual need is to know the start/stop state of the queue. That is for app to 
decide if 'rte_eth_tx_done_cleanup()' can be done or not an a queue:
https://patches.dpdk.org/project/dpdk/patch/1614938252-62955-1-git-send-email-oulijun@huawei.com/

And normally I also prefer APIs with simple & clear responsibility, but this one 
seems very related to the existing '_queue_info_get()' ones, so I am fine with 
both options.

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 3/23/2021 10:13 AM, Ferruh Yigit wrote:
> On 3/22/2021 6:53 PM, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Sent: Monday, March 22, 2021 5:08 PM
>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh 
>>> <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
>>> thomas@monjalon.net
>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko 
>>> <arybchenko@solarflare.com>; David Marchand
>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi 
>>> <bluca@debian.org>
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue 
>>> information
>>>
>>> On 3/22/21 7:53 PM, Ananyev, Konstantin wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> Sent: Monday, March 22, 2021 4:02 PM
>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh 
>>>>> <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>;
>>>>> thomas@monjalon.net
>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko 
>>>>> <arybchenko@solarflare.com>; David Marchand
>>>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi 
>>>>> <bluca@debian.org>
>>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue 
>>>>> information
>>>>>
>>>>> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>>>>>>> Sent: Monday, March 22, 2021 2:49 PM
>>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou 
>>>>>>> <oulijun@huawei.com>; thomas@monjalon.net
>>>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko 
>>>>>>> <arybchenko@solarflare.com>; David Marchand
>>>>>>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi 
>>>>>>> <bluca@debian.org>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve 
>>>>>>> queue information
>>>>>>>
>>>>>>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
>>>>>>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>>>>>>>>> Currently, upper-layer application could get queue state only
>>>>>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>>>>>>> this is not the recommended way to access it. So this patch
>>>>>>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>>>>>>> rte_eth_tx_queue_info_get API.
>>>>>>>>>
>>>>>>>>> Note: The hairpin queue is not supported with above
>>>>>>>>> rte_eth_*x_queue_info_get, so the queue state could be
>>>>>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>>>>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>>>>>>> it could be ABI compatible.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>
>>>>>>>> <...>
>>>>>>>>
>>>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>>>>>>> index efda313..3b83c5a 100644
>>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>>>>>>>>        uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>>>>>>        uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>>>>>>        uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>>>>> +    uint8_t queue_state;
>>>>>>>>>    } __rte_cache_min_aligned;
>>>>>>>>>      /**
>>>>>>>>> @@ -1600,6 +1602,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. */
>>>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>>>>>>> +    uint8_t queue_state;
>>>>>>>>>    } __rte_cache_min_aligned;
>>>>>>>>>      /* Generic Burst mode flag definition, values can be ORed. */
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is causing an ABI warning [1], but I guess it is safe since the
>>>>>>>> size of the struct is not changing (cache align). Adding a few more
>>>>>>>> people to comment.
>>>>>>>>
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
>>>>>>>
>>>>>>> Frankly speaking I dislike addition of queue_state as uint8_t.
>>>>>>> IMHO it should be either 'bool started' or enum to support more
>>>>>>> states in the future if we need.
>>>>>>
>>>>>> I think we already have set of defines for it:
>>>>>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define 
>>>>>> RTE_ETH_QUEUE_STATE_STOPPED 0
>>>>>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define 
>>>>>> RTE_ETH_QUEUE_STATE_STARTED 1
>>>>>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define 
>>>>>> RTE_ETH_QUEUE_STATE_HAIRPIN 2
>>>>>>
>>>>>> If we want to publish it, then might be enough just move these macros to 
>>>>>> rte_ethdev.h or so.
>>>>>>
>>>>>> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
>>>>>> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array 
>>>>>> of uint8_t.
>>>>>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
>>>>>> Unless in future will want to change it in struct rte_eth_dev_data too
>>>>>> (or even hide it inside dev private queue data).
>>>>>
>>>>> I forgot about hairpin and bitmask... If so, I think it is
>>>>> sufficient to fix absolutely misleading comment, say
>>>>> that it is a bit mask and think about removal of
>>>>> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
>>>>> stopped+hairpin). May be consider to use uin16_t,
>>>>> since 8 bit is really small bitmask. It still fits in
>>>>> available hole.
>>>>
>>>> Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
>>>> and each of the states (stopped/started/hairpin) is mutually exclusive.
>>>> Is that not what was intended when hairpin queues were introduced?
>>>>
>>>
>>> Thanks, yes, you're right. My memory lies to me. If queue state
>>> is not a bit mask, it should be an enum from API point of view.
>>> Rx/Tx queue info structures are control path. I see no point to
>>> save bits here. Clear API is more important on control path.
>>> The only reason here to use uint8_t is to avoid ABI breakage.
>>> I can't judge if it is critical to wait or not.
>>
>> As alternate thought - introduce new API function,
>> something like:
>> int rte_eth_get_rxq_state(portid, queue_id);
>> Then rte_eth_dev_is_rx_hairpin_queue() probably can be deprecated
>> in favour of this new one.
>>
>>
> 
> The 'rte_eth_dev_is_rx_hairpin_queue()' is internal function, and it is not 
> visible to the application, it should be OK to keep it.
> 
> But 'STATE_HAIRPIN' should be kept internal, or should be available to the 
> application?
> 
> The actual need is to know the start/stop state of the queue. That is for app to 
> decide if 'rte_eth_tx_done_cleanup()' can be done or not an a queue:
> https://patches.dpdk.org/project/dpdk/patch/1614938252-62955-1-git-send-email-oulijun@huawei.com/ 
> 
> 
> And normally I also prefer APIs with simple & clear responsibility, but this one 
> seems very related to the existing '_queue_info_get()' ones, so I am fine with 
> both options.
> 

Another high-level discussion is, testpmd keeps lots of config/state itself, I 
assume that is because it is not possible to get all DPDK config/state from DPDK 
library, but not sure if this is a design decision.

Should we try to provide all config/state information via DPDK APIs, or should 
we push this responsibility to the application level?

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> >>>>>>>>
> >>>>>>>> Note: The hairpin queue is not supported with above
> >>>>>>>> rte_eth_*x_queue_info_get, so the queue state could be
> >>>>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >>>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>>>>>>> it could be ABI compatible.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>>>>>
> >>>>>>> <...>
> >>>>>>>
> >>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>> b/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>> index efda313..3b83c5a 100644
> >>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
> >>>>>>>>        uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>>>>>>>        uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>>>>>>        uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>>>>>> +    uint8_t queue_state;
> >>>>>>>>    } __rte_cache_min_aligned;
> >>>>>>>>      /**
> >>>>>>>> @@ -1600,6 +1602,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. */
> >>>>>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>>>>>> +    uint8_t queue_state;
> >>>>>>>>    } __rte_cache_min_aligned;
> >>>>>>>>      /* Generic Burst mode flag definition, values can be ORed. */
> >>>>>>>>
> >>>>>>>
> >>>>>>> This is causing an ABI warning [1], but I guess it is safe since the
> >>>>>>> size of the struct is not changing (cache align). Adding a few more
> >>>>>>> people to comment.
> >>>>>>>
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> >>>>>>
> >>>>>> Frankly speaking I dislike addition of queue_state as uint8_t.
> >>>>>> IMHO it should be either 'bool started' or enum to support more
> >>>>>> states in the future if we need.
> >>>>>
> >>>>> I think we already have set of defines for it:
> >>>>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> >>>>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> >>>>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> >>>>>
> >>>>> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> >>>>>
> >>>>> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> >>>>> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> >>>>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> >>>>> Unless in future will want to change it in struct rte_eth_dev_data too
> >>>>> (or even hide it inside dev private queue data).
> >>>>
> >>>> I forgot about hairpin and bitmask... If so, I think it is
> >>>> sufficient to fix absolutely misleading comment, say
> >>>> that it is a bit mask and think about removal of
> >>>> RTE_ETH_QUEUE_STATE_STOPPED (since it could be
> >>>> stopped+hairpin). May be consider to use uin16_t,
> >>>> since 8 bit is really small bitmask. It still fits in
> >>>> available hole.
> >>>
> >>> Hmm, as I can read the code - hairpin queue can't be started/stopped by SW,
> >>> and each of the states (stopped/started/hairpin) is mutually exclusive.
> >>> Is that not what was intended when hairpin queues were introduced?
> >>>
> >>
> >> Thanks, yes, you're right. My memory lies to me. If queue state
> >> is not a bit mask, it should be an enum from API point of view.
> >> Rx/Tx queue info structures are control path. I see no point to
> >> save bits here. Clear API is more important on control path.
> >> The only reason here to use uint8_t is to avoid ABI breakage.
> >> I can't judge if it is critical to wait or not.
> >
> > As alternate thought - introduce new API function,
> > something like:
> > int rte_eth_get_rxq_state(portid, queue_id);
> > Then rte_eth_dev_is_rx_hairpin_queue() probably can be deprecated
> > in favour of this new one.
> >
> >
> 
> The 'rte_eth_dev_is_rx_hairpin_queue()' is internal function, and it is not
> visible to the application, it should be OK to keep it.

What I am saying - we well have get-state() - PMDs can use the new one
instead of  rte_eth_dev_is_rx_hairpin_queue().
Or rte_eth_dev_is_rx_hairpin_queue() can be just a wrapper around get_rxq_state().

> 
> But 'STATE_HAIRPIN' should be kept internal, or should be available to the
> application?
> 
> The actual need is to know the start/stop state of the queue. That is for app to
> decide if 'rte_eth_tx_done_cleanup()' can be done or not an a queue:
> https://patches.dpdk.org/project/dpdk/patch/1614938252-62955-1-git-send-email-oulijun@huawei.com/

If we don't expose STATE_HAIRPIN what state we will report 
for hairpin queue back to the user?
Either STARTED or STOPPED are both invalid and misleading.
I  think we have to report all 3 supported states back to the user.

> 
> And normally I also prefer APIs with simple & clear responsibility, but this one
> seems very related to the existing '_queue_info_get()' ones, so I am fine with
> both options.

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/3/22 23:45, Ananyev, Konstantin 写道:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>> Sent: Monday, March 22, 2021 2:49 PM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
>> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
>>
>> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
>>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
>>>> Currently, upper-layer application could get queue state only
>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>> this is not the recommended way to access it. So this patch
>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>> rte_eth_tx_queue_info_get API.
>>>>
>>>> Note: The hairpin queue is not supported with above
>>>> rte_eth_*x_queue_info_get, so the queue state could be
>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>> it could be ABI compatible.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>
>>> <...>
>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>> index efda313..3b83c5a 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
>>>>        uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>        uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>        uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>> +    uint8_t queue_state;
>>>>    } __rte_cache_min_aligned;
>>>>      /**
>>>> @@ -1600,6 +1602,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. */
>>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
>>>> +    uint8_t queue_state;
>>>>    } __rte_cache_min_aligned;
>>>>      /* Generic Burst mode flag definition, values can be ORed. */
>>>>
>>>
>>> This is causing an ABI warning [1], but I guess it is safe since the
>>> size of the struct is not changing (cache align). Adding a few more
>>> people to comment.
>>>
>>>
>>> [1]
>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
>>
>> Frankly speaking I dislike addition of queue_state as uint8_t.
>> IMHO it should be either 'bool started' or enum to support more
>> states in the future if we need.
> 
> I think we already have set of defines for it:
> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
At the latest date, the rte_ethdev_driver.h file does not exist.
> 
> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> 
> About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> Unless in future will want to change it in struct rte_eth_dev_data too
> (or even hide it inside dev private queue data).
>    
> 
> 

Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> 
> 
> 在 2021/3/22 23:45, Ananyev, Konstantin 写道:
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> >> Sent: Monday, March 22, 2021 2:49 PM
> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko <arybchenko@solarflare.com>; David Marchand
> >> <david.marchand@redhat.com>; Ray Kinsella <mdr@ashroe.eu>; Luca Boccassi <bluca@debian.org>
> >> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information
> >>
> >> On 3/22/21 12:22 PM, Ferruh Yigit wrote:
> >>> On 3/18/2021 12:25 PM, Lijun Ou wrote:
> >>>> Currently, upper-layer application could get queue state only
> >>>> through pointers such as dev->data->tx_queue_state[queue_id],
> >>>> this is not the recommended way to access it. So this patch
> >>>> add get queue state when call rte_eth_rx_queue_info_get and
> >>>> rte_eth_tx_queue_info_get API.
> >>>>
> >>>> Note: The hairpin queue is not supported with above
> >>>> rte_eth_*x_queue_info_get, so the queue state could be
> >>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>>> it could be ABI compatible.
> >>>>
> >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>
> >>> <...>
> >>>
> >>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>> b/lib/librte_ethdev/rte_ethdev.h
> >>>> index efda313..3b83c5a 100644
> >>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info {
> >>>>        uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>>>        uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>>        uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>> +    uint8_t queue_state;
> >>>>    } __rte_cache_min_aligned;
> >>>>      /**
> >>>> @@ -1600,6 +1602,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. */
> >>>> +    /**< Queues state: STARTED(1) / STOPPED(0). */
> >>>> +    uint8_t queue_state;
> >>>>    } __rte_cache_min_aligned;
> >>>>      /* Generic Burst mode flag definition, values can be ORed. */
> >>>>
> >>>
> >>> This is causing an ABI warning [1], but I guess it is safe since the
> >>> size of the struct is not changing (cache align). Adding a few more
> >>> people to comment.
> >>>
> >>>
> >>> [1]
> >>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651
> >>
> >> Frankly speaking I dislike addition of queue_state as uint8_t.
> >> IMHO it should be either 'bool started' or enum to support more
> >> states in the future if we need.
> >
> > I think we already have set of defines for it:
> > lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0
> > lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1
> > lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> At the latest date, the rte_ethdev_driver.h file does not exist.

Yep, It was renamed to ethdev_driver.h.
But the defines are still there.

> >
> > If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so.
> >
> > About uint8_t vs enum - yes, in principle enum would be a bit nicer,
> > but right now rte_eth_dev_data.(rx|tx)_queue_state[]  itself is an array of uint8_t.
> > So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info.
> > Unless in future will want to change it in struct rte_eth_dev_data too
> > (or even hide it inside dev private queue data).
> >
> >
> >

[dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: The hairpin queue is not supported with above
rte_eth_*x_queue_info_get, so the queue state could be
RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V1->V2:
- move queue state defines to public file
---
 doc/guides/rel_notes/release_21_05.rst |  6 ++++++
 lib/librte_ethdev/ethdev_driver.h      |  7 -------
 lib/librte_ethdev/rte_ethdev.c         |  3 +++
 lib/librte_ethdev/rte_ethdev.h         | 11 +++++++++++
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 22aa80a..503daf9 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -164,6 +164,12 @@ ABI Changes
 
 * No ABI change that would break compatibility with 20.11.
 
+* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
+  to provide indicated rxq queue state.
+
+* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
+  to provide indicated txq queue state.
+
 
 Known Issues
 ------------
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index cdd4b43..ec5a17d 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -970,13 +970,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 3059aa5..fbd10b2 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index efda313..4f0b1b2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1582,6 +1582,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
+	/**< Queues state: STARTED(1) / STOPPED(0). */
+	uint8_t queue_state;
 } __rte_cache_min_aligned;
 
 /**
@@ -1600,6 +1609,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. */
+	/**< Queues state: STARTED(1) / STOPPED(0). */
+	uint8_t queue_state;
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[oulijun]

Hi, all,
     any comments for this patch?
     Hope for your reply.

Thanks

在 2021/3/25 19:09, Lijun Ou 写道:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: The hairpin queue is not supported with above
> rte_eth_*x_queue_info_get, so the queue state could be
> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V1->V2:
> - move queue state defines to public file
> ---
>   doc/guides/rel_notes/release_21_05.rst |  6 ++++++
>   lib/librte_ethdev/ethdev_driver.h      |  7 -------
>   lib/librte_ethdev/rte_ethdev.c         |  3 +++
>   lib/librte_ethdev/rte_ethdev.h         | 11 +++++++++++
>   4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> index 22aa80a..503daf9 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -164,6 +164,12 @@ ABI Changes
>   
>   * No ABI change that would break compatibility with 20.11.
>   
> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> +  to provide indicated rxq queue state.
> +
> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> +  to provide indicated txq queue state.
> +
>   
>   Known Issues
>   ------------
> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> index cdd4b43..ec5a17d 100644
> --- a/lib/librte_ethdev/ethdev_driver.h
> +++ b/lib/librte_ethdev/ethdev_driver.h
> @@ -970,13 +970,6 @@ struct eth_dev_ops {
>   };
>   
>   /**
> - * RX/TX queue states
> - */
> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> -#define RTE_ETH_QUEUE_STATE_STARTED 1
> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> -
> -/**
>    * @internal
>    * Check if the selected Rx queue is hairpin queue.
>    *
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3059aa5..fbd10b2 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   
>   	memset(qinfo, 0, sizeof(*qinfo));
>   	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> +
>   	return 0;
>   }
>   
> @@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   
>   	memset(qinfo, 0, sizeof(*qinfo));
>   	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
>   
>   	return 0;
>   }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index efda313..4f0b1b2 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1582,6 +1582,13 @@ struct rte_eth_dev_info {
>   };
>   
>   /**
> + * RX/TX queue states
> + */
> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> +#define RTE_ETH_QUEUE_STATE_STARTED 1
> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> +
> +/**
>    * Ethernet device RX queue information structure.
>    * Used to retrieve information about configured queue.
>    */
> @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
>   	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>   	uint16_t nb_desc;           /**< configured number of RXDs. */
>   	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */
> +	uint8_t queue_state;
>   } __rte_cache_min_aligned;
>   
>   /**
> @@ -1600,6 +1609,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. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */
> +	uint8_t queue_state;
>   } __rte_cache_min_aligned;
>   
>   /* Generic Burst mode flag definition, values can be ORed. */
> 

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Stephen Hemminger]

On Tue, 6 Apr 2021 08:49:04 +0800
oulijun <oulijun@huawei.com> wrote:

> >   /**
> > + * RX/TX queue states
> > + */
> > +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> > +#define RTE_ETH_QUEUE_STATE_STARTED 1
> > +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2

These could be an enum?

> > +/**
> >    * Ethernet device RX queue information structure.
> >    * Used to retrieve information about configured queue.
> >    */
> > @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
> >   	uint8_t scattered_rx;       /**< scattered packets RX supported. */

There is a one byte hole here waiting to be used.
Why not use that?

> >   	uint16_t nb_desc;           /**< configured number of RXDs. */
> >   	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> > +	/**< Queues state: STARTED(1) / STOPPED(0). */
> > +	uint8_t queue_state;
> >   } __rte_cache_min_aligned;

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/6/2021 2:55 AM, Stephen Hemminger wrote:
> On Tue, 6 Apr 2021 08:49:04 +0800
> oulijun <oulijun@huawei.com> wrote:
> 
>>>    /**
>>> + * RX/TX queue states
>>> + */
>>> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
>>> +#define RTE_ETH_QUEUE_STATE_STARTED 1
>>> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> 
> These could be an enum?
> 

These values are already used to assign to 
'rte_eth_dev_data->[rt]x_queue_state', which are 'uint8_t', end we can't change 
them to 'enum' because of ABI concerns, so I think we can keep them as it is.

>>> +/**
>>>     * Ethernet device RX queue information structure.
>>>     * Used to retrieve information about configured queue.
>>>     */
>>> @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
>>>    	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> 
> There is a one byte hole here waiting to be used.
> Why not use that?
> 

+1

>>>    	uint16_t nb_desc;           /**< configured number of RXDs. */
>>>    	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>> +	/**< Queues state: STARTED(1) / STOPPED(0). */
>>> +	uint8_t queue_state;
>>>    } __rte_cache_min_aligned;

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

Hi,

> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: The hairpin queue is not supported with above
> rte_eth_*x_queue_info_get, so the queue state could be
> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.

I wonder why RTE_ETH_QUEUE_STATE_HAIRPIN Is not supported?
Obviously what we do - copy internal queue state to the user provided buffer.

> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V1->V2:
> - move queue state defines to public file
> ---
>  doc/guides/rel_notes/release_21_05.rst |  6 ++++++
>  lib/librte_ethdev/ethdev_driver.h      |  7 -------
>  lib/librte_ethdev/rte_ethdev.c         |  3 +++
>  lib/librte_ethdev/rte_ethdev.h         | 11 +++++++++++
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> index 22aa80a..503daf9 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -164,6 +164,12 @@ ABI Changes
> 
>  * No ABI change that would break compatibility with 20.11.
> 
> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> +  to provide indicated rxq queue state.
> +
> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> +  to provide indicated txq queue state.
> +
> 
>  Known Issues
>  ------------
> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> index cdd4b43..ec5a17d 100644
> --- a/lib/librte_ethdev/ethdev_driver.h
> +++ b/lib/librte_ethdev/ethdev_driver.h
> @@ -970,13 +970,6 @@ struct eth_dev_ops {
>  };
> 
>  /**
> - * RX/TX queue states
> - */
> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> -#define RTE_ETH_QUEUE_STATE_STARTED 1
> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> -
> -/**
>   * @internal
>   * Check if the selected Rx queue is hairpin queue.
>   *
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3059aa5..fbd10b2 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> 
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> +
>  	return 0;
>  }
> 
> @@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> 
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
> 
>  	return 0;
>  }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index efda313..4f0b1b2 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1582,6 +1582,13 @@ struct rte_eth_dev_info {
>  };
> 
>  /**
> + * RX/TX queue states
> + */
> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> +#define RTE_ETH_QUEUE_STATE_STARTED 1
> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> +
> +/**
>   * Ethernet device RX queue information structure.
>   * Used to retrieve information about configured queue.
>   */
> @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>  	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */

I think comment has to state that possible values are one of
RTE_ETH_QUEUE_STATE_*.
About previous discussion about new field in this struct vs new API function,
I still think new function will be a bit cleaner, but could live with both.

> +	uint8_t queue_state;

If we'll go with new 1B field, then as Stephen pointed,
it is probably worth to fill the hole between scattered_rx
and nb_desc with this new filed.

>  } __rte_cache_min_aligned;
> 
>  /**
> @@ -1600,6 +1609,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. */
> +	/**< Queues state: STARTED(1) / STOPPED(0). */

Same about comment here.

> +	uint8_t queue_state;
>  } __rte_cache_min_aligned;
> 
>  /* Generic Burst mode flag definition, values can be ORed. */
> --
> 2.7.4

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

Hi Lijun,

Let's try to complete this for the release.

On 4/6/2021 3:02 PM, Ananyev, Konstantin wrote:
> Hi,
> 
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: The hairpin queue is not supported with above
>> rte_eth_*x_queue_info_get, so the queue state could be
>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> 
> I wonder why RTE_ETH_QUEUE_STATE_HAIRPIN Is not supported?
> Obviously what we do - copy internal queue state to the user provided buffer.
> 

+1, with current implementation we can't say it is only for start & stop.

Since 'STATE_HAIRPIN' is all internal, it may be possible to separate it into 
its own variable and expose only start and stop, but I don't think it worth the 
effort, why not just expose all possible states.


>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V1->V2:
>> - move queue state defines to public file
>> ---
>>   doc/guides/rel_notes/release_21_05.rst |  6 ++++++
>>   lib/librte_ethdev/ethdev_driver.h      |  7 -------
>>   lib/librte_ethdev/rte_ethdev.c         |  3 +++
>>   lib/librte_ethdev/rte_ethdev.h         | 11 +++++++++++
>>   4 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
>> index 22aa80a..503daf9 100644
>> --- a/doc/guides/rel_notes/release_21_05.rst
>> +++ b/doc/guides/rel_notes/release_21_05.rst
>> @@ -164,6 +164,12 @@ ABI Changes
>>
>>   * No ABI change that would break compatibility with 20.11.
>>
>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
>> +  to provide indicated rxq queue state.
>> +
>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
>> +  to provide indicated txq queue state.
>> +
>>
>>   Known Issues
>>   ------------
>> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
>> index cdd4b43..ec5a17d 100644
>> --- a/lib/librte_ethdev/ethdev_driver.h
>> +++ b/lib/librte_ethdev/ethdev_driver.h
>> @@ -970,13 +970,6 @@ struct eth_dev_ops {
>>   };
>>
>>   /**
>> - * RX/TX queue states
>> - */
>> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
>> -#define RTE_ETH_QUEUE_STATE_STARTED 1
>> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>> -
>> -/**
>>    * @internal
>>    * Check if the selected Rx queue is hairpin queue.
>>    *
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 3059aa5..fbd10b2 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>
>>   memset(qinfo, 0, sizeof(*qinfo));
>>   dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
>> +qinfo->queue_state = dev->data->rx_queue_state[queue_id];
>> +
>>   return 0;
>>   }
>>
>> @@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>
>>   memset(qinfo, 0, sizeof(*qinfo));
>>   dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
>> +qinfo->queue_state = dev->data->tx_queue_state[queue_id];
>>
>>   return 0;
>>   }
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index efda313..4f0b1b2 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1582,6 +1582,13 @@ struct rte_eth_dev_info {
>>   };
>>
>>   /**
>> + * RX/TX queue states
>> + */
>> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
>> +#define RTE_ETH_QUEUE_STATE_STARTED 1
>> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>> +
>> +/**
>>    * Ethernet device RX queue information structure.
>>    * Used to retrieve information about configured queue.
>>    */
>> @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
>>   uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>   uint16_t nb_desc;           /**< configured number of RXDs. */
>>   uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>> +/**< Queues state: STARTED(1) / STOPPED(0). */
> 
> I think comment has to state that possible values are one of
> RTE_ETH_QUEUE_STATE_*.
> About previous discussion about new field in this struct vs new API function,
> I still think new function will be a bit cleaner, but could live with both.
> 
>> +uint8_t queue_state;
> 
> If we'll go with new 1B field, then as Stephen pointed,
> it is probably worth to fill the hole between scattered_rx
> and nb_desc with this new filed.
> 

+1

>>   } __rte_cache_min_aligned;
>>
>>   /**
>> @@ -1600,6 +1609,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. */
>> +/**< Queues state: STARTED(1) / STOPPED(0). */
> 
> Same about comment here.
> 
>> +uint8_t queue_state;
>>   } __rte_cache_min_aligned;
>>
>>   /* Generic Burst mode flag definition, values can be ORed. */
>> --
>> 2.7.4
> 


Other comments I case see:

1- Make QUEUE_STATE enum
   For consistency with existing usage I think we can keep it as it is

2- Make a specific API to get the queue state
   No strong opinion, I think we can go with this one

3- Use enum type in "struct rte_eth_rxq_info"
   Which make sense but we don't have space in current struct, also 
'rte_eth_dev_data' has variable to hold same, and for consistency if we change 
it to enum in it, that is even wider change. I think it doesn't worth the effort 
and we can continue with 'uint8_t'

Please add if any is missing, and if there is any strong opinion on above.


If there is no objection, only required changes are above two issues commented 
inline,
- Remove comment/note that this is only for start/stop states
- Replace the field location to benefit from gap in struct

Re: [dpdk-dev] [PATCH V2] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> Hi Lijun,
> 
> Let's try to complete this for the release.
> 
> On 4/6/2021 3:02 PM, Ananyev, Konstantin wrote:
> > Hi,
> >
> >> Currently, upper-layer application could get queue state only
> >> through pointers such as dev->data->tx_queue_state[queue_id],
> >> this is not the recommended way to access it. So this patch
> >> add get queue state when call rte_eth_rx_queue_info_get and
> >> rte_eth_tx_queue_info_get API.
> >>
> >> Note: The hairpin queue is not supported with above
> >> rte_eth_*x_queue_info_get, so the queue state could be
> >> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED.
> >
> > I wonder why RTE_ETH_QUEUE_STATE_HAIRPIN Is not supported?
> > Obviously what we do - copy internal queue state to the user provided buffer.
> >
> 
> +1, with current implementation we can't say it is only for start & stop.
> 
> Since 'STATE_HAIRPIN' is all internal, it may be possible to separate it into

With this patch - not any more, as we move RTE_ETH_QUEUE_STATE_* Defines
into rte_ethdev.h. 

> its own variable and expose only start and stop, but I don't think it worth the
> effort, why not just expose all possible states.
> 
> 
> >> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >> it could be ABI compatible.
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >> ---
> >> V1->V2:
> >> - move queue state defines to public file
> >> ---
> >>   doc/guides/rel_notes/release_21_05.rst |  6 ++++++
> >>   lib/librte_ethdev/ethdev_driver.h      |  7 -------
> >>   lib/librte_ethdev/rte_ethdev.c         |  3 +++
> >>   lib/librte_ethdev/rte_ethdev.h         | 11 +++++++++++
> >>   4 files changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> >> index 22aa80a..503daf9 100644
> >> --- a/doc/guides/rel_notes/release_21_05.rst
> >> +++ b/doc/guides/rel_notes/release_21_05.rst
> >> @@ -164,6 +164,12 @@ ABI Changes
> >>
> >>   * No ABI change that would break compatibility with 20.11.
> >>
> >> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> >> +  to provide indicated rxq queue state.
> >> +
> >> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> >> +  to provide indicated txq queue state.
> >> +
> >>
> >>   Known Issues
> >>   ------------
> >> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> >> index cdd4b43..ec5a17d 100644
> >> --- a/lib/librte_ethdev/ethdev_driver.h
> >> +++ b/lib/librte_ethdev/ethdev_driver.h
> >> @@ -970,13 +970,6 @@ struct eth_dev_ops {
> >>   };
> >>
> >>   /**
> >> - * RX/TX queue states
> >> - */
> >> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> >> -#define RTE_ETH_QUEUE_STATE_STARTED 1
> >> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> >> -
> >> -/**
> >>    * @internal
> >>    * Check if the selected Rx queue is hairpin queue.
> >>    *
> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >> index 3059aa5..fbd10b2 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >>
> >>   memset(qinfo, 0, sizeof(*qinfo));
> >>   dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> >> +qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> >> +
> >>   return 0;
> >>   }
> >>
> >> @@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >>
> >>   memset(qinfo, 0, sizeof(*qinfo));
> >>   dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> >> +qinfo->queue_state = dev->data->tx_queue_state[queue_id];
> >>
> >>   return 0;
> >>   }
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >> index efda313..4f0b1b2 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1582,6 +1582,13 @@ struct rte_eth_dev_info {
> >>   };
> >>
> >>   /**
> >> + * RX/TX queue states
> >> + */
> >> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> >> +#define RTE_ETH_QUEUE_STATE_STARTED 1
> >> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> >> +
> >> +/**
> >>    * Ethernet device RX queue information structure.
> >>    * Used to retrieve information about configured queue.
> >>    */
> >> @@ -1591,6 +1598,8 @@ struct rte_eth_rxq_info {
> >>   uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>   uint16_t nb_desc;           /**< configured number of RXDs. */
> >>   uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >> +/**< Queues state: STARTED(1) / STOPPED(0). */
> >
> > I think comment has to state that possible values are one of
> > RTE_ETH_QUEUE_STATE_*.
> > About previous discussion about new field in this struct vs new API function,
> > I still think new function will be a bit cleaner, but could live with both.
> >
> >> +uint8_t queue_state;
> >
> > If we'll go with new 1B field, then as Stephen pointed,
> > it is probably worth to fill the hole between scattered_rx
> > and nb_desc with this new filed.
> >
> 
> +1
> 
> >>   } __rte_cache_min_aligned;
> >>
> >>   /**
> >> @@ -1600,6 +1609,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. */
> >> +/**< Queues state: STARTED(1) / STOPPED(0). */
> >
> > Same about comment here.
> >
> >> +uint8_t queue_state;
> >>   } __rte_cache_min_aligned;
> >>
> >>   /* Generic Burst mode flag definition, values can be ORed. */
> >> --
> >> 2.7.4
> >
> 
> 
> Other comments I case see:
> 
> 1- Make QUEUE_STATE enum
>    For consistency with existing usage I think we can keep it as it is
> 
> 2- Make a specific API to get the queue state
>    No strong opinion, I think we can go with this one
> 
> 3- Use enum type in "struct rte_eth_rxq_info"
>    Which make sense but we don't have space in current struct, also
> 'rte_eth_dev_data' has variable to hold same, and for consistency if we change
> it to enum in it, that is even wider change. I think it doesn't worth the effort
> and we can continue with 'uint8_t'
> 
> Please add if any is missing, and if there is any strong opinion on above.
> 
> 
> If there is no objection, only required changes are above two issues commented
> inline,
> - Remove comment/note that this is only for start/stop states
> - Replace the field location to benefit from gap in struct

Sounds good to me.
Konstantin

[dpdk-dev] [PATCH V3] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V2->V3:
- rewrite the commit log and delete the part Note
- rewrite tht comments for queue state
- move the queue_state definition locations

V1->V2:
- move queue state defines to public file
---
 doc/guides/rel_notes/release_21_05.rst | 6 ++++++
 lib/librte_ethdev/ethdev_driver.h      | 7 -------
 lib/librte_ethdev/rte_ethdev.c         | 3 +++
 lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 3bd7757..c6e45e2 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -251,6 +251,12 @@ ABI Changes
   function was already marked as internal in the API documentation for it,
   and was not for use by external applications.
 
+* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
+  to provide indicated rxq queue state.
+
+* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
+  to provide indicated txq queue state.
+
 
 Known Issues
 ------------
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index 113129d..40e474a 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -952,13 +952,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 6b5cfd6..ab188ec 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..a0d01d2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;
@@ -1606,6 +1614,7 @@ 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. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH V3] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/15/2021 3:40 AM, Lijun Ou wrote:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

Looks good to me, but it is causing an ABI error as we already discussed before 
as that is false positive.


Ray, David,

Should we add any exception to libabigail.abignore for this?

Re: [dpdk-dev] [PATCH V3] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

15/04/2021 14:33, Ferruh Yigit:
> On 4/15/2021 3:40 AM, Lijun Ou wrote:
> > Currently, upper-layer application could get queue state only
> > through pointers such as dev->data->tx_queue_state[queue_id],
> > this is not the recommended way to access it. So this patch
> > add get queue state when call rte_eth_rx_queue_info_get and
> > rte_eth_tx_queue_info_get API.
> > 
> > Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> > remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> > it could be ABI compatible.
> > 
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > Signed-off-by: Lijun Ou <oulijun@huawei.com>
> 
> Looks good to me, but it is causing an ABI error as we already discussed before 
> as that is false positive.
> 
> 
> Ray, David,
> 
> Should we add any exception to libabigail.abignore for this?

We do not tolerate any ABI warning.
If we are sure the ABI change is false positive,
it must be suppressed in libabigail.abignore in the same patch.

Re: [dpdk-dev] [PATCH V3] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/15/2021 1:36 PM, Thomas Monjalon wrote:
> 15/04/2021 14:33, Ferruh Yigit:
>> On 4/15/2021 3:40 AM, Lijun Ou wrote:
>>> Currently, upper-layer application could get queue state only
>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>> this is not the recommended way to access it. So this patch
>>> add get queue state when call rte_eth_rx_queue_info_get and
>>> rte_eth_tx_queue_info_get API.
>>>
>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>> it could be ABI compatible.
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>
>> Looks good to me, but it is causing an ABI error as we already discussed before
>> as that is false positive.
>>
>>
>> Ray, David,
>>
>> Should we add any exception to libabigail.abignore for this?
> 
> We do not tolerate any ABI warning.
> If we are sure the ABI change is false positive,
> it must be suppressed in libabigail.abignore in the same patch.
> 

A new field is added to public structs, but struct size or the location of the 
existing fields are not changing (struct is cache aligned).

Can you please support how this can be added to 'libabigail.abignore'?

Lijun can you please send a new version with 'libabigail.abignore' update?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH V3] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

15/04/2021 14:45, Ferruh Yigit:
> On 4/15/2021 1:36 PM, Thomas Monjalon wrote:
> > 15/04/2021 14:33, Ferruh Yigit:
> >> On 4/15/2021 3:40 AM, Lijun Ou wrote:
> >>> Currently, upper-layer application could get queue state only
> >>> through pointers such as dev->data->tx_queue_state[queue_id],
> >>> this is not the recommended way to access it. So this patch
> >>> add get queue state when call rte_eth_rx_queue_info_get and
> >>> rte_eth_tx_queue_info_get API.
> >>>
> >>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>> it could be ABI compatible.
> >>>
> >>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>
> >> Looks good to me, but it is causing an ABI error as we already discussed before
> >> as that is false positive.
> >>
> >> Ray, David,
> >>
> >> Should we add any exception to libabigail.abignore for this?
> > 
> > We do not tolerate any ABI warning.
> > If we are sure the ABI change is false positive,
> > it must be suppressed in libabigail.abignore in the same patch.
> 
> A new field is added to public structs, but struct size or the location of the 
> existing fields are not changing (struct is cache aligned).
> 
> Can you please support how this can be added to 'libabigail.abignore'?

This is how you can check the struct sizes (in 32 and 64-bit builds):

for lib in ethdev ; do for struct in rte_eth_{r,t}xq_info ; do for build in dpdk-build/build-{32b,x86-generic} ; do basename $build ; pahole -sC $struct $build/lib/librte_$lib.a ; done ; done ; done

I confirm the additions are not changing the ABI.
And because they are provided as output infos,
there is no issue like using potentially unitialized holes.

It must be explicited in libabigail with this syntax:
https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppress-type

; Ignore fields inserted in alignment hole of rte_eth_rxq_info
[suppress_type]
        name = rte_eth_rxq_info
        has_data_member_inserted_at = offset_after(scattered_rx)

; Ignore fields inserted in cacheline boundary of rte_eth_txq_info
[suppress_type]
        name = rte_eth_txq_info
        has_data_member_inserted_between = {offset_after(nb_desc), end}

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V3] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/4/15 20:45, Ferruh Yigit 写道:
> On 4/15/2021 1:36 PM, Thomas Monjalon wrote:
>> 15/04/2021 14:33, Ferruh Yigit:
>>> On 4/15/2021 3:40 AM, Lijun Ou wrote:
>>>> Currently, upper-layer application could get queue state only
>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>> this is not the recommended way to access it. So this patch
>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>> rte_eth_tx_queue_info_get API.
>>>>
>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>> it could be ABI compatible.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>
>>> Looks good to me, but it is causing an ABI error as we already 
>>> discussed before
>>> as that is false positive.
>>>
>>>
>>> Ray, David,
>>>
>>> Should we add any exception to libabigail.abignore for this?
>>
>> We do not tolerate any ABI warning.
>> If we are sure the ABI change is false positive,
>> it must be suppressed in libabigail.abignore in the same patch.
>>
> 
> A new field is added to public structs, but struct size or the location 
> of the existing fields are not changing (struct is cache aligned).
> 
> Can you please support how this can be added to 'libabigail.abignore'?
> 
> Lijun can you please send a new version with 'libabigail.abignore' update?
> 
Yes, I can do that. But at the moment I don't know how to update 
libabigil.abinore. I don't know where to modify
Is there any relevant documentation?
> Thanks,
> ferruh
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V3] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/16/2021 1:58 AM, oulijun wrote:
> 
> 
> 在 2021/4/15 20:45, Ferruh Yigit 写道:
>> On 4/15/2021 1:36 PM, Thomas Monjalon wrote:
>>> 15/04/2021 14:33, Ferruh Yigit:
>>>> On 4/15/2021 3:40 AM, Lijun Ou wrote:
>>>>> Currently, upper-layer application could get queue state only
>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>>> this is not the recommended way to access it. So this patch
>>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>>> rte_eth_tx_queue_info_get API.
>>>>>
>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>>> it could be ABI compatible.
>>>>>
>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>
>>>> Looks good to me, but it is causing an ABI error as we already discussed before
>>>> as that is false positive.
>>>>
>>>>
>>>> Ray, David,
>>>>
>>>> Should we add any exception to libabigail.abignore for this?
>>>
>>> We do not tolerate any ABI warning.
>>> If we are sure the ABI change is false positive,
>>> it must be suppressed in libabigail.abignore in the same patch.
>>>
>>
>> A new field is added to public structs, but struct size or the location of the 
>> existing fields are not changing (struct is cache aligned).
>>
>> Can you please support how this can be added to 'libabigail.abignore'?
>>
>> Lijun can you please send a new version with 'libabigail.abignore' update?
>>
> Yes, I can do that. But at the moment I don't know how to update 
> libabigil.abinore. I don't know where to modify
> Is there any relevant documentation?

Please check Thomas' response, he already described what needs to be changed.

And for documentation David & Ray may know better.

[dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 devtools/libabigail.abignore           | 12 +++++++++++-
 doc/guides/rel_notes/release_21_05.rst |  6 ++++++
 lib/librte_ethdev/ethdev_driver.h      |  7 -------
 lib/librte_ethdev/rte_ethdev.c         |  3 +++
 lib/librte_ethdev/rte_ethdev.h         |  9 +++++++++
 5 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 6c0b389..54542a8 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -19,4 +19,14 @@
 ; Ignore fields inserted in cacheline boundary of rte_cryptodev
 [suppress_type]
         name = rte_cryptodev
-        has_data_member_inserted_between = {offset_after(attached), end}
\ No newline at end of file
+        has_data_member_inserted_between = {offset_after(attached), end}
+
+; Ignore fields inserted in alignment hole of rte_eth_rxq_info
+[suppress_type]
+        name = rte_eth_rxq_info
+        has_data_member_inserted_at = offset_after(scattered_rx)
+
+; Ignore fields inserted in cacheline boundary of rte_eth_txq_info
+[suppress_type]
+        name = rte_eth_txq_info
+        has_data_member_inserted_between = {offset_after(nb_desc), end}
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 3bd7757..c6e45e2 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -251,6 +251,12 @@ ABI Changes
   function was already marked as internal in the API documentation for it,
   and was not for use by external applications.
 
+* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
+  to provide indicated rxq queue state.
+
+* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
+  to provide indicated txq queue state.
+
 
 Known Issues
 ------------
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index 113129d..40e474a 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -952,13 +952,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 6b5cfd6..ab188ec 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..a0d01d2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;
@@ -1606,6 +1614,7 @@ 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. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

16/04/2021 10:46, Lijun Ou:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
[...]
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -251,6 +251,12 @@ ABI Changes
>    function was already marked as internal in the API documentation for it,
>    and was not for use by external applications.
>  
> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> +  to provide indicated rxq queue state.
> +
> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> +  to provide indicated txq queue state.

Not sure we should add a note here for additions which
do not break ABI compatibility.
It may be confusing.

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/16/2021 9:58 AM, Thomas Monjalon wrote:
> 16/04/2021 10:46, Lijun Ou:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
> [...]
>> --- a/doc/guides/rel_notes/release_21_05.rst
>> +++ b/doc/guides/rel_notes/release_21_05.rst
>> @@ -251,6 +251,12 @@ ABI Changes
>>     function was already marked as internal in the API documentation for it,
>>     and was not for use by external applications.
>>   
>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
>> +  to provide indicated rxq queue state.
>> +
>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
>> +  to provide indicated txq queue state.
> 
> Not sure we should add a note here for additions which
> do not break ABI compatibility.
> It may be confusing.
> 

Hi Thomas,

What do about adding the documentation to "API Changes" section?
Since 'rte_eth_rx_queue_info_get()'/'rte_eth_tx_queue_info_get()' can get 
'queue_state' now, which may taken as API change.

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

16/04/2021 11:41, Ferruh Yigit:
> On 4/16/2021 9:58 AM, Thomas Monjalon wrote:
> > 16/04/2021 10:46, Lijun Ou:
> >> Currently, upper-layer application could get queue state only
> >> through pointers such as dev->data->tx_queue_state[queue_id],
> >> this is not the recommended way to access it. So this patch
> >> add get queue state when call rte_eth_rx_queue_info_get and
> >> rte_eth_tx_queue_info_get API.
> >>
> >> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >> it could be ABI compatible.
> > [...]
> >> --- a/doc/guides/rel_notes/release_21_05.rst
> >> +++ b/doc/guides/rel_notes/release_21_05.rst
> >> @@ -251,6 +251,12 @@ ABI Changes
> >>     function was already marked as internal in the API documentation for it,
> >>     and was not for use by external applications.
> >>   
> >> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> >> +  to provide indicated rxq queue state.
> >> +
> >> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> >> +  to provide indicated txq queue state.
> > 
> > Not sure we should add a note here for additions which
> > do not break ABI compatibility.
> > It may be confusing.
> > 
> 
> Hi Thomas,
> 
> What do about adding the documentation to "API Changes" section?
> Since 'rte_eth_rx_queue_info_get()'/'rte_eth_tx_queue_info_get()' can get 
> 'queue_state' now, which may taken as API change.

That's an addition.
The users have nothing to change in their existing code,
so I think we don't need a note in API or ABI change.
The only required note would be in the "New Features".

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Kinsella, Ray]

On 16/04/2021 10:57, Thomas Monjalon wrote:
> 16/04/2021 11:41, Ferruh Yigit:
>> On 4/16/2021 9:58 AM, Thomas Monjalon wrote:
>>> 16/04/2021 10:46, Lijun Ou:
>>>> Currently, upper-layer application could get queue state only
>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>> this is not the recommended way to access it. So this patch
>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>> rte_eth_tx_queue_info_get API.
>>>>
>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>> it could be ABI compatible.
>>> [...]
>>>> --- a/doc/guides/rel_notes/release_21_05.rst
>>>> +++ b/doc/guides/rel_notes/release_21_05.rst
>>>> @@ -251,6 +251,12 @@ ABI Changes
>>>>     function was already marked as internal in the API documentation for it,
>>>>     and was not for use by external applications.
>>>>   
>>>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
>>>> +  to provide indicated rxq queue state.
>>>> +
>>>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
>>>> +  to provide indicated txq queue state.
>>>
>>> Not sure we should add a note here for additions which
>>> do not break ABI compatibility.
>>> It may be confusing.
>>>
>>
>> Hi Thomas,
>>
>> What do about adding the documentation to "API Changes" section?
>> Since 'rte_eth_rx_queue_info_get()'/'rte_eth_tx_queue_info_get()' can get 
>> 'queue_state' now, which may taken as API change.
> 
> That's an addition.
> The users have nothing to change in their existing code,
> so I think we don't need a note in API or ABI change.
> The only required note would be in the "New Features".

Well it definitely isn't an ABI change, however it still is an API addition.
I don't know, if additions qualify as changes.

Ray K
 

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

Kinsella, Ray:
> On 16/04/2021 10:57, Thomas Monjalon wrote:
> > 16/04/2021 11:41, Ferruh Yigit:
> >> On 4/16/2021 9:58 AM, Thomas Monjalon wrote:
> >>> 16/04/2021 10:46, Lijun Ou:
> >>>> Currently, upper-layer application could get queue state only
> >>>> through pointers such as dev->data->tx_queue_state[queue_id],
> >>>> this is not the recommended way to access it. So this patch
> >>>> add get queue state when call rte_eth_rx_queue_info_get and
> >>>> rte_eth_tx_queue_info_get API.
> >>>>
> >>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> >>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> >>>> it could be ABI compatible.
> >>> [...]
> >>>> --- a/doc/guides/rel_notes/release_21_05.rst
> >>>> +++ b/doc/guides/rel_notes/release_21_05.rst
> >>>> @@ -251,6 +251,12 @@ ABI Changes
> >>>>     function was already marked as internal in the API documentation for it,
> >>>>     and was not for use by external applications.
> >>>>   
> >>>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> >>>> +  to provide indicated rxq queue state.
> >>>> +
> >>>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> >>>> +  to provide indicated txq queue state.
> >>>
> >>> Not sure we should add a note here for additions which
> >>> do not break ABI compatibility.
> >>> It may be confusing.
> >>>
> >>
> >> Hi Thomas,
> >>
> >> What do about adding the documentation to "API Changes" section?
> >> Since 'rte_eth_rx_queue_info_get()'/'rte_eth_tx_queue_info_get()' can get 
> >> 'queue_state' now, which may taken as API change.
> > 
> > That's an addition.
> > The users have nothing to change in their existing code,
> > so I think we don't need a note in API or ABI change.
> > The only required note would be in the "New Features".
> 
> Well it definitely isn't an ABI change, however it still is an API addition.
> I don't know, if additions qualify as changes.

Additions are already notified in the section "New Features" in general.
The purpose of the API section in the release notes is for app developers
to be warned of changes requiring attention in their maintenance.

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Kinsella, Ray]

On 25/04/2021 17:42, Thomas Monjalon wrote:
> Kinsella, Ray:
>> On 16/04/2021 10:57, Thomas Monjalon wrote:
>>> 16/04/2021 11:41, Ferruh Yigit:
>>>> On 4/16/2021 9:58 AM, Thomas Monjalon wrote:
>>>>> 16/04/2021 10:46, Lijun Ou:
>>>>>> Currently, upper-layer application could get queue state only
>>>>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>>>>> this is not the recommended way to access it. So this patch
>>>>>> add get queue state when call rte_eth_rx_queue_info_get and
>>>>>> rte_eth_tx_queue_info_get API.
>>>>>>
>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>>>>> it could be ABI compatible.
>>>>> [...]
>>>>>> --- a/doc/guides/rel_notes/release_21_05.rst
>>>>>> +++ b/doc/guides/rel_notes/release_21_05.rst
>>>>>> @@ -251,6 +251,12 @@ ABI Changes
>>>>>>     function was already marked as internal in the API documentation for it,
>>>>>>     and was not for use by external applications.
>>>>>>   
>>>>>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
>>>>>> +  to provide indicated rxq queue state.
>>>>>> +
>>>>>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
>>>>>> +  to provide indicated txq queue state.
>>>>>
>>>>> Not sure we should add a note here for additions which
>>>>> do not break ABI compatibility.
>>>>> It may be confusing.
>>>>>
>>>>
>>>> Hi Thomas,
>>>>
>>>> What do about adding the documentation to "API Changes" section?
>>>> Since 'rte_eth_rx_queue_info_get()'/'rte_eth_tx_queue_info_get()' can get 
>>>> 'queue_state' now, which may taken as API change.
>>>
>>> That's an addition.
>>> The users have nothing to change in their existing code,
>>> so I think we don't need a note in API or ABI change.
>>> The only required note would be in the "New Features".
>>
>> Well it definitely isn't an ABI change, however it still is an API addition.
>> I don't know, if additions qualify as changes.
> 
> Additions are already notified in the section "New Features" in general.
> The purpose of the API section in the release notes is for app developers
> to be warned of changes requiring attention in their maintenance.
> 

Understood - thanks.

Ray K

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/4/16 16:58, Thomas Monjalon 写道:
> 16/04/2021 10:46, Lijun Ou:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
> [...]
>> --- a/doc/guides/rel_notes/release_21_05.rst
>> +++ b/doc/guides/rel_notes/release_21_05.rst
>> @@ -251,6 +251,12 @@ ABI Changes
>>     function was already marked as internal in the API documentation for it,
>>     and was not for use by external applications.
>>   
>> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
>> +  to provide indicated rxq queue state.
>> +
>> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
>> +  to provide indicated txq queue state.
> 
> Not sure we should add a note here for additions which
> do not break ABI compatibility.
> It may be confusing.
> 
Hi，Thmas&Ferruh
	Do you want to delete it?
> 
> .
> 

Re: [dpdk-dev] [PATCH V4] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

 
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>  devtools/libabigail.abignore           | 12 +++++++++++-
>  doc/guides/rel_notes/release_21_05.rst |  6 ++++++
>  lib/librte_ethdev/ethdev_driver.h      |  7 -------
>  lib/librte_ethdev/rte_ethdev.c         |  3 +++
>  lib/librte_ethdev/rte_ethdev.h         |  9 +++++++++
>  5 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 6c0b389..54542a8 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -19,4 +19,14 @@
>  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
>  [suppress_type]
>          name = rte_cryptodev
> -        has_data_member_inserted_between = {offset_after(attached), end}
> \ No newline at end of file
> +        has_data_member_inserted_between = {offset_after(attached), end}
> +
> +; Ignore fields inserted in alignment hole of rte_eth_rxq_info
> +[suppress_type]
> +        name = rte_eth_rxq_info
> +        has_data_member_inserted_at = offset_after(scattered_rx)
> +
> +; Ignore fields inserted in cacheline boundary of rte_eth_txq_info
> +[suppress_type]
> +        name = rte_eth_txq_info
> +        has_data_member_inserted_between = {offset_after(nb_desc), end}
> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> index 3bd7757..c6e45e2 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -251,6 +251,12 @@ ABI Changes
>    function was already marked as internal in the API documentation for it,
>    and was not for use by external applications.
> 
> +* Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure
> +  to provide indicated rxq queue state.
> +
> +* Added new field ``queue_state`` to ``rte_eth_txq_info`` structure
> +  to provide indicated txq queue state.
> +
> 
>  Known Issues
>  ------------
> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> index 113129d..40e474a 100644
> --- a/lib/librte_ethdev/ethdev_driver.h
> +++ b/lib/librte_ethdev/ethdev_driver.h
> @@ -952,13 +952,6 @@ struct eth_dev_ops {
>  };
> 
>  /**
> - * RX/TX queue states
> - */
> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> -#define RTE_ETH_QUEUE_STATE_STARTED 1
> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> -
> -/**
>   * @internal
>   * Check if the selected Rx queue is hairpin queue.
>   *
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 6b5cfd6..ab188ec 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5042,6 +5042,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> 
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> +
>  	return 0;
>  }
> 
> @@ -5082,6 +5084,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> 
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
> 
>  	return 0;
>  }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 3b773b6..a0d01d2 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
>  };
> 
>  /**
> + * RX/TX queue states
> + */
> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> +#define RTE_ETH_QUEUE_STATE_STARTED 1
> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> +
> +/**
>   * Ethernet device RX queue information structure.
>   * Used to retrieve information about configured queue.
>   */
> @@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
>  	struct rte_mempool *mp;     /**< mempool used by that queue. */
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>  	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>  } __rte_cache_min_aligned;
> @@ -1606,6 +1614,7 @@ 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. */
> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>  } __rte_cache_min_aligned;
> 
>  /* Generic Burst mode flag definition, values can be ORed. */
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.7.4

[dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
V4->V5:
- Add acked-by
- add a note to the "New features" section to annouce the new feature.

V3->V4:
- update libabigail.abignore for removing the CI warnings

V2->V3:
- rewrite the commit log and delete the part Note
- rewrite tht comments for queue state
- move the queue_state definition locations

V1->V2:
- move queue state defines to public file
---
 doc/guides/rel_notes/release_21_05.rst | 6 ++++++
 lib/librte_ethdev/ethdev_driver.h      | 7 -------
 lib/librte_ethdev/rte_ethdev.c         | 3 +++
 lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 58272e1..1ab3681 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -81,6 +81,12 @@ New Features
       representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
       representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
 
+* **Enhanced function for getting rxq/txq info ABI.**
+  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
+    provide indicated rxq queue state.
+  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
+    provide indicated txq queue state.
+
 * **Added support for meter PPS profile.**
 
   Currently meter algorithms only supports bytes per second(BPS).
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index 113129d..40e474a 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -952,13 +952,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index c73d263..d5adf4f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..a0d01d2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;
@@ -1606,6 +1614,7 @@ 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. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/17/2021 4:09 AM, Lijun Ou wrote:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> V4->V5:
> - Add acked-by
> - add a note to the "New features" section to annouce the new feature.
> 
> V3->V4:
> - update libabigail.abignore for removing the CI warnings
> 
> V2->V3:
> - rewrite the commit log and delete the part Note
> - rewrite tht comments for queue state
> - move the queue_state definition locations
> 
> V1->V2:
> - move queue state defines to public file
> ---
>   doc/guides/rel_notes/release_21_05.rst | 6 ++++++
>   lib/librte_ethdev/ethdev_driver.h      | 7 -------
>   lib/librte_ethdev/rte_ethdev.c         | 3 +++
>   lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
>   4 files changed, 18 insertions(+), 7 deletions(-)

missing 'libabigail.abignore' that was in v4?

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/4/18 6:00, Ferruh Yigit 写道:
> On 4/17/2021 4:09 AM, Lijun Ou wrote:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> V4->V5:
>> - Add acked-by
>> - add a note to the "New features" section to annouce the new feature.
>>
>> V3->V4:
>> - update libabigail.abignore for removing the CI warnings
>>
>> V2->V3:
>> - rewrite the commit log and delete the part Note
>> - rewrite tht comments for queue state
>> - move the queue_state definition locations
>>
>> V1->V2:
>> - move queue state defines to public file
>> ---
>>   doc/guides/rel_notes/release_21_05.rst | 6 ++++++
>>   lib/librte_ethdev/ethdev_driver.h      | 7 -------
>>   lib/librte_ethdev/rte_ethdev.c         | 3 +++
>>   lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
>>   4 files changed, 18 insertions(+), 7 deletions(-)
> 
> missing 'libabigail.abignore' that was in v4?
> .
Yes. the CI is warning after I send the V3 with fix some comments.
Then I update liabigail.bignore, taking into account your comments with 
thomas and send the V4.
> 

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/4/18 6:00, Ferruh Yigit 写道:
> On 4/17/2021 4:09 AM, Lijun Ou wrote:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> V4->V5:
>> - Add acked-by
>> - add a note to the "New features" section to annouce the new feature.
>>
>> V3->V4:
>> - update libabigail.abignore for removing the CI warnings
>>
>> V2->V3:
>> - rewrite the commit log and delete the part Note
>> - rewrite tht comments for queue state
>> - move the queue_state definition locations
>>
>> V1->V2:
>> - move queue state defines to public file
>> ---
>>   doc/guides/rel_notes/release_21_05.rst | 6 ++++++
>>   lib/librte_ethdev/ethdev_driver.h      | 7 -------
>>   lib/librte_ethdev/rte_ethdev.c         | 3 +++
>>   lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
>>   4 files changed, 18 insertions(+), 7 deletions(-)
> 
> missing 'libabigail.abignore' that was in v4?
> .
Sorry. thanks. I have sent the new version for V6.
> 

[dpdk-dev] [PATCH V6] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
V5->V6:
- add updated update libabigail.abignore

V4->V5:
- add acked-by
- add a note to the "New features" section to annouce the new feature.

V3->V4:
- update libabigail.abignore for removing the CI warnings

V2->V3:
- rewrite the commit log and delete the part Note
- rewrite tht comments for queue state
- move the queue_state definition locations

V1->V2:
- move queue state defines to public file
---
 devtools/libabigail.abignore           | 12 +++++++++++-
 doc/guides/rel_notes/release_21_05.rst |  6 ++++++
 lib/librte_ethdev/ethdev_driver.h      |  7 -------
 lib/librte_ethdev/rte_ethdev.c         |  3 +++
 lib/librte_ethdev/rte_ethdev.h         |  9 +++++++++
 5 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 6c0b389..d8082cf 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -19,4 +19,14 @@
 ; Ignore fields inserted in cacheline boundary of rte_cryptodev
 [suppress_type]
         name = rte_cryptodev
-        has_data_member_inserted_between = {offset_after(attached), end}
\ No newline at end of file
+        has_data_member_inserted_between = {offset_after(attached), end}
+
+; Ignore fields inserted in alignment hole of rte_eth_rxq_info
+[suppress_type]
+	name = rte_eth_rxq_info
+	has_data_member_inserted_at = offset_after(scattered_rx)
+
+; Ignore fields inserted in cacheline boundary of rte_eth_txq_info
+[suppress_type]
+	name = rte_eth_txq_info
+	has_data_member_inserted_between = {offset_after(nb_desc), end}
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 58272e1..1ab3681 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -81,6 +81,12 @@ New Features
       representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
       representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
 
+* **Enhanced function for getting rxq/txq info ABI.**
+  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
+    provide indicated rxq queue state.
+  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
+    provide indicated txq queue state.
+
 * **Added support for meter PPS profile.**
 
   Currently meter algorithms only supports bytes per second(BPS).
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index 113129d..40e474a 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -952,13 +952,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index c73d263..d5adf4f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..a0d01d2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;
@@ -1606,6 +1614,7 @@ 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. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH V6] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

19/04/2021 04:03, Lijun Ou:
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> +* **Enhanced function for getting rxq/txq info ABI.**

Reword: Added queue state in queried Rx/Tx queue info

> +  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
> +    provide indicated rxq queue state.

s/rxq queue/Rx queue/

> +  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
> +    provide indicated txq queue state.

s/txq queue/Tx queue/

Thanks

Re: [dpdk-dev] [PATCH V6] ethdev: add queue state when retrieve queue information

[oulijun]

在 2021/4/19 16:41, Thomas Monjalon 写道:
> 19/04/2021 04:03, Lijun Ou:
>> --- a/doc/guides/rel_notes/release_21_05.rst
>> +++ b/doc/guides/rel_notes/release_21_05.rst
>> +* **Enhanced function for getting rxq/txq info ABI.**
> 
> Reword: Added queue state in queried Rx/Tx queue info
> 
>> +  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
>> +    provide indicated rxq queue state.
> 
> s/rxq queue/Rx queue/
> 
>> +  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
>> +    provide indicated txq queue state.
> 
> s/txq queue/Tx queue/
> 
> Thanks
OK，I will fix it in the next version.
> 
> 
> .
> 

[dpdk-dev] [PATCH v7] ethdev: add queue state when retrieve queue information

[Lijun Ou]

Currently, upper-layer application could get queue state only
through pointers such as dev->data->tx_queue_state[queue_id],
this is not the recommended way to access it. So this patch
add get queue state when call rte_eth_rx_queue_info_get and
rte_eth_tx_queue_info_get API.

Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
it could be ABI compatible.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
V6->V7:
- reword the note title for "New Feature"
- use Rx/Tx queue instead of rxq/txq queue

V5->V6:
- add updated update libabigail.abignore

V4->V5:
- add acked-by
- add a note to the "New features" section to annouce the new feature.

V3->V4:
- update libabigail.abignore for removing the CI warnings

V2->V3:
- rewrite the commit log and delete the part Note
- rewrite tht comments for queue state
- move the queue_state definition locations

V1->V2:
- move queue state defines to public file
---
 devtools/libabigail.abignore           | 12 +++++++++++-
 doc/guides/rel_notes/release_21_05.rst |  6 ++++++
 lib/librte_ethdev/ethdev_driver.h      |  7 -------
 lib/librte_ethdev/rte_ethdev.c         |  3 +++
 lib/librte_ethdev/rte_ethdev.h         |  9 +++++++++
 5 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 6c0b389..d8082cf 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -19,4 +19,14 @@
 ; Ignore fields inserted in cacheline boundary of rte_cryptodev
 [suppress_type]
         name = rte_cryptodev
-        has_data_member_inserted_between = {offset_after(attached), end}
\ No newline at end of file
+        has_data_member_inserted_between = {offset_after(attached), end}
+
+; Ignore fields inserted in alignment hole of rte_eth_rxq_info
+[suppress_type]
+	name = rte_eth_rxq_info
+	has_data_member_inserted_at = offset_after(scattered_rx)
+
+; Ignore fields inserted in cacheline boundary of rte_eth_txq_info
+[suppress_type]
+	name = rte_eth_txq_info
+	has_data_member_inserted_between = {offset_after(nb_desc), end}
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 58272e1..05b2f0c 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -81,6 +81,12 @@ New Features
       representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
       representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
 
+* **Added queue state in queried Rx/Tx queue info.**
+  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
+    provide indicated Rx queue state.
+  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
+    provide indicated Tx queue state.
+
 * **Added support for meter PPS profile.**
 
   Currently meter algorithms only supports bytes per second(BPS).
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
index 113129d..40e474a 100644
--- a/lib/librte_ethdev/ethdev_driver.h
+++ b/lib/librte_ethdev/ethdev_driver.h
@@ -952,13 +952,6 @@ struct eth_dev_ops {
 };
 
 /**
- * RX/TX queue states
- */
-#define RTE_ETH_QUEUE_STATE_STOPPED 0
-#define RTE_ETH_QUEUE_STATE_STARTED 1
-#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
-
-/**
  * @internal
  * Check if the selected Rx queue is hairpin queue.
  *
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index c73d263..d5adf4f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
+
 	return 0;
 }
 
@@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
+	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
 
 	return 0;
 }
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..a0d01d2 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
 };
 
 /**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
+
+/**
  * Ethernet device RX queue information structure.
  * Used to retrieve information about configured queue.
  */
@@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;
@@ -1606,6 +1614,7 @@ 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. */
+	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
 } __rte_cache_min_aligned;
 
 /* Generic Burst mode flag definition, values can be ORed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v7] ethdev: add queue state when retrieve queue information

[Thomas Monjalon]

19/04/2021 10:57, Lijun Ou:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [dpdk-dev] [PATCH v7] ethdev: add queue state when retrieve queue information

[Ferruh Yigit]

On 4/19/2021 10:03 AM, Thomas Monjalon wrote:
> 19/04/2021 10:57, Lijun Ou:
>> Currently, upper-layer application could get queue state only
>> through pointers such as dev->data->tx_queue_state[queue_id],
>> this is not the recommended way to access it. So this patch
>> add get queue state when call rte_eth_rx_queue_info_get and
>> rte_eth_tx_queue_info_get API.
>>
>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>> it could be ABI compatible.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[Kinsella, Ray]

On 17/04/2021 04:09, Lijun Ou wrote:
> Currently, upper-layer application could get queue state only
> through pointers such as dev->data->tx_queue_state[queue_id],
> this is not the recommended way to access it. So this patch
> add get queue state when call rte_eth_rx_queue_info_get and
> rte_eth_tx_queue_info_get API.
> 
> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> it could be ABI compatible.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> V4->V5:
> - Add acked-by
> - add a note to the "New features" section to annouce the new feature.
> 
> V3->V4:
> - update libabigail.abignore for removing the CI warnings
> 
> V2->V3:
> - rewrite the commit log and delete the part Note
> - rewrite tht comments for queue state
> - move the queue_state definition locations
> 
> V1->V2:
> - move queue state defines to public file
> ---
>  doc/guides/rel_notes/release_21_05.rst | 6 ++++++
>  lib/librte_ethdev/ethdev_driver.h      | 7 -------
>  lib/librte_ethdev/rte_ethdev.c         | 3 +++
>  lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> index 58272e1..1ab3681 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -81,6 +81,12 @@ New Features
>        representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
>        representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
>  
> +* **Enhanced function for getting rxq/txq info ABI.**
> +  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
> +    provide indicated rxq queue state.
> +  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
> +    provide indicated txq queue state.
> +
>  * **Added support for meter PPS profile.**
>  
>    Currently meter algorithms only supports bytes per second(BPS).
> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> index 113129d..40e474a 100644
> --- a/lib/librte_ethdev/ethdev_driver.h
> +++ b/lib/librte_ethdev/ethdev_driver.h
> @@ -952,13 +952,6 @@ struct eth_dev_ops {
>  };
>  
>  /**
> - * RX/TX queue states
> - */
> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> -#define RTE_ETH_QUEUE_STATE_STARTED 1
> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> -
> -/**
>   * @internal
>   * Check if the selected Rx queue is hairpin queue.
>   *
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index c73d263..d5adf4f 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>  
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> +
>  	return 0;
>  }
>  
> @@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>  
>  	memset(qinfo, 0, sizeof(*qinfo));
>  	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
>  
>  	return 0;
>  }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 3b773b6..a0d01d2 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
>  };
>  
>  /**
> + * RX/TX queue states
> + */
> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> +#define RTE_ETH_QUEUE_STATE_STARTED 1
> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> +
> +/**
>   * Ethernet device RX queue information structure.
>   * Used to retrieve information about configured queue.
>   */
> @@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
>  	struct rte_mempool *mp;     /**< mempool used by that queue. */
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */

Are we sure this is a false positive? - it is being added mid-structure on the rx-side at least.
Shouldn't this be appended to the end - unless it is being sneaked into padding between fields. 

>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>  	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>  } __rte_cache_min_aligned;
> @@ -1606,6 +1614,7 @@ 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. */
> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>  } __rte_cache_min_aligned;
>  
>  /* Generic Burst mode flag definition, values can be ORed. */
> 

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kinsella, Ray
> Sent: Friday, April 23, 2021 12:17 PM
> To: Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information
> 
> 
> 
> On 17/04/2021 04:09, Lijun Ou wrote:
> > Currently, upper-layer application could get queue state only
> > through pointers such as dev->data->tx_queue_state[queue_id],
> > this is not the recommended way to access it. So this patch
> > add get queue state when call rte_eth_rx_queue_info_get and
> > rte_eth_tx_queue_info_get API.
> >
> > Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
> > remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
> > it could be ABI compatible.
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > Signed-off-by: Lijun Ou <oulijun@huawei.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> > V4->V5:
> > - Add acked-by
> > - add a note to the "New features" section to annouce the new feature.
> >
> > V3->V4:
> > - update libabigail.abignore for removing the CI warnings
> >
> > V2->V3:
> > - rewrite the commit log and delete the part Note
> > - rewrite tht comments for queue state
> > - move the queue_state definition locations
> >
> > V1->V2:
> > - move queue state defines to public file
> > ---
> >  doc/guides/rel_notes/release_21_05.rst | 6 ++++++
> >  lib/librte_ethdev/ethdev_driver.h      | 7 -------
> >  lib/librte_ethdev/rte_ethdev.c         | 3 +++
> >  lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
> >  4 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
> > index 58272e1..1ab3681 100644
> > --- a/doc/guides/rel_notes/release_21_05.rst
> > +++ b/doc/guides/rel_notes/release_21_05.rst
> > @@ -81,6 +81,12 @@ New Features
> >        representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
> >        representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
> >
> > +* **Enhanced function for getting rxq/txq info ABI.**
> > +  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
> > +    provide indicated rxq queue state.
> > +  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
> > +    provide indicated txq queue state.
> > +
> >  * **Added support for meter PPS profile.**
> >
> >    Currently meter algorithms only supports bytes per second(BPS).
> > diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> > index 113129d..40e474a 100644
> > --- a/lib/librte_ethdev/ethdev_driver.h
> > +++ b/lib/librte_ethdev/ethdev_driver.h
> > @@ -952,13 +952,6 @@ struct eth_dev_ops {
> >  };
> >
> >  /**
> > - * RX/TX queue states
> > - */
> > -#define RTE_ETH_QUEUE_STATE_STOPPED 0
> > -#define RTE_ETH_QUEUE_STATE_STARTED 1
> > -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> > -
> > -/**
> >   * @internal
> >   * Check if the selected Rx queue is hairpin queue.
> >   *
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index c73d263..d5adf4f 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >
> >  	memset(qinfo, 0, sizeof(*qinfo));
> >  	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> > +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
> > +
> >  	return 0;
> >  }
> >
> > @@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >
> >  	memset(qinfo, 0, sizeof(*qinfo));
> >  	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
> > +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
> >
> >  	return 0;
> >  }
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index 3b773b6..a0d01d2 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
> >  };
> >
> >  /**
> > + * RX/TX queue states
> > + */
> > +#define RTE_ETH_QUEUE_STATE_STOPPED 0
> > +#define RTE_ETH_QUEUE_STATE_STARTED 1
> > +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> > +
> > +/**
> >   * Ethernet device RX queue information structure.
> >   * Used to retrieve information about configured queue.
> >   */
> > @@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
> >  	struct rte_mempool *mp;     /**< mempool used by that queue. */
> >  	struct rte_eth_rxconf conf; /**< queue config parameters. */
> >  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> > +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
> 
> Are we sure this is a false positive? - it is being added mid-structure on the rx-side at least.
> Shouldn't this be appended to the end - unless it is being sneaked into padding between fields.

I believe there was a padding, that's why it was suggested to squeeze it here.

> 
> >  	uint16_t nb_desc;           /**< configured number of RXDs. */
> >  	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
> >  } __rte_cache_min_aligned;
> > @@ -1606,6 +1614,7 @@ 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. */
> > +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
> >  } __rte_cache_min_aligned;
> >
> >  /* Generic Burst mode flag definition, values can be ORed. */
> >

Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information

[Kinsella, Ray]

On 23/04/2021 12:26, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Kinsella, Ray
>> Sent: Friday, April 23, 2021 12:17 PM
>> To: Lijun Ou <oulijun@huawei.com>; thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: Re: [dpdk-dev] [PATCH V5] ethdev: add queue state when retrieve queue information
>>
>>
>>
>> On 17/04/2021 04:09, Lijun Ou wrote:
>>> Currently, upper-layer application could get queue state only
>>> through pointers such as dev->data->tx_queue_state[queue_id],
>>> this is not the recommended way to access it. So this patch
>>> add get queue state when call rte_eth_rx_queue_info_get and
>>> rte_eth_tx_queue_info_get API.
>>>
>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size
>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so
>>> it could be ABI compatible.
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> ---
>>> V4->V5:
>>> - Add acked-by
>>> - add a note to the "New features" section to annouce the new feature.
>>>
>>> V3->V4:
>>> - update libabigail.abignore for removing the CI warnings
>>>
>>> V2->V3:
>>> - rewrite the commit log and delete the part Note
>>> - rewrite tht comments for queue state
>>> - move the queue_state definition locations
>>>
>>> V1->V2:
>>> - move queue state defines to public file
>>> ---
>>>  doc/guides/rel_notes/release_21_05.rst | 6 ++++++
>>>  lib/librte_ethdev/ethdev_driver.h      | 7 -------
>>>  lib/librte_ethdev/rte_ethdev.c         | 3 +++
>>>  lib/librte_ethdev/rte_ethdev.h         | 9 +++++++++
>>>  4 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
>>> index 58272e1..1ab3681 100644
>>> --- a/doc/guides/rel_notes/release_21_05.rst
>>> +++ b/doc/guides/rel_notes/release_21_05.rst
>>> @@ -81,6 +81,12 @@ New Features
>>>        representor=[[c#]pf#]sf# sf[0,2-1023] /* 1023 SFs.                     */
>>>        representor=[c#]pf#      c2pf[0,1]    /* 2 PFs on controller 2.        */
>>>
>>> +* **Enhanced function for getting rxq/txq info ABI.**
>>> +  * Added new field ``queue_state`` to ``rte_eth_rxq_info`` structure to
>>> +    provide indicated rxq queue state.
>>> +  * Added new field ``queue_state`` to ``rte_eth_txq_info`` structure to
>>> +    provide indicated txq queue state.
>>> +
>>>  * **Added support for meter PPS profile.**
>>>
>>>    Currently meter algorithms only supports bytes per second(BPS).
>>> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
>>> index 113129d..40e474a 100644
>>> --- a/lib/librte_ethdev/ethdev_driver.h
>>> +++ b/lib/librte_ethdev/ethdev_driver.h
>>> @@ -952,13 +952,6 @@ struct eth_dev_ops {
>>>  };
>>>
>>>  /**
>>> - * RX/TX queue states
>>> - */
>>> -#define RTE_ETH_QUEUE_STATE_STOPPED 0
>>> -#define RTE_ETH_QUEUE_STATE_STARTED 1
>>> -#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>>> -
>>> -/**
>>>   * @internal
>>>   * Check if the selected Rx queue is hairpin queue.
>>>   *
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index c73d263..d5adf4f 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -5038,6 +5038,8 @@ rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>>
>>>  	memset(qinfo, 0, sizeof(*qinfo));
>>>  	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
>>> +	qinfo->queue_state = dev->data->rx_queue_state[queue_id];
>>> +
>>>  	return 0;
>>>  }
>>>
>>> @@ -5078,6 +5080,7 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>>
>>>  	memset(qinfo, 0, sizeof(*qinfo));
>>>  	dev->dev_ops->txq_info_get(dev, queue_id, qinfo);
>>> +	qinfo->queue_state = dev->data->tx_queue_state[queue_id];
>>>
>>>  	return 0;
>>>  }
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 3b773b6..a0d01d2 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1588,6 +1588,13 @@ struct rte_eth_dev_info {
>>>  };
>>>
>>>  /**
>>> + * RX/TX queue states
>>> + */
>>> +#define RTE_ETH_QUEUE_STATE_STOPPED 0
>>> +#define RTE_ETH_QUEUE_STATE_STARTED 1
>>> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
>>> +
>>> +/**
>>>   * Ethernet device RX queue information structure.
>>>   * Used to retrieve information about configured queue.
>>>   */
>>> @@ -1595,6 +1602,7 @@ struct rte_eth_rxq_info {
>>>  	struct rte_mempool *mp;     /**< mempool used by that queue. */
>>>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>>
>> Are we sure this is a false positive? - it is being added mid-structure on the rx-side at least.
>> Shouldn't this be appended to the end - unless it is being sneaked into padding between fields.
> 
> I believe there was a padding, that's why it was suggested to squeeze it here.

Yes - I took a look with pahole after reading back in the thread a bit,
where Thomas points out the 1byte padding hole in memory we could use.
Good work, as it avoids having to rework the structure at the next ABI version. 

>>
>>>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>>>  	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
>>>  } __rte_cache_min_aligned;
>>> @@ -1606,6 +1614,7 @@ 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. */
>>> +	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>>>  } __rte_cache_min_aligned;
>>>
>>>  /* Generic Burst mode flag definition, values can be ORed. */
>>>