> -----Original Message----- > From: Andrew Rybchenko <[email protected]> > Sent: Monday, March 22, 2021 5:08 PM > To: Ananyev, Konstantin <[email protected]>; Yigit, Ferruh > <[email protected]>; Lijun Ou <[email protected]>; > [email protected] > Cc: [email protected]; [email protected]; Andrew Rybchenko > <[email protected]>; David Marchand > <[email protected]>; Ray Kinsella <[email protected]>; Luca Boccassi > <[email protected]> > 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 <[email protected]> > >> Sent: Monday, March 22, 2021 4:02 PM > >> To: Ananyev, Konstantin <[email protected]>; Yigit, Ferruh > >> <[email protected]>; Lijun Ou <[email protected]>; > >> [email protected] > >> Cc: [email protected]; [email protected]; Andrew Rybchenko > >> <[email protected]>; David Marchand > >> <[email protected]>; Ray Kinsella <[email protected]>; Luca Boccassi > >> <[email protected]> > >> 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 <[email protected]> On Behalf Of Andrew Rybchenko > >>>> Sent: Monday, March 22, 2021 2:49 PM > >>>> To: Yigit, Ferruh <[email protected]>; Lijun Ou > >>>> <[email protected]>; [email protected] > >>>> Cc: [email protected]; [email protected]; Andrew Rybchenko > >>>> <[email protected]>; David Marchand > >>>> <[email protected]>; Ray Kinsella <[email protected]>; Luca > >>>> Boccassi <[email protected]> > >>>> 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 <[email protected]> > >>>>>> Signed-off-by: Lijun Ou <[email protected]> > >>>>> > >>>>> <...> > >>>>> > >>>>>> 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.
