ethdev: add queue state when retrieve queue information
Checks
Commit Message
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(+)
Comments
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
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
在 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
> .
>
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.
> -----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).
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.
> -----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?
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.
> -----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.
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.
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?
> >>>>>>>>
> >>>>>>>> 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.
在 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).
>
>
>
>
>
> 在 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).
> >
> >
> >
@@ -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
------------
@@ -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;
}
@@ -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. */