[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
On 20/10/2015 09:53, Qiu, Michael wrote: > But as I know it is different all the time, am I right? > If yes, I don't know what's the value of this field. It can be used to get some snapshot/instant view informations while we have to monitor and debug.
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
On 2015/10/20 16:09, Vincent JARDIN wrote: > On 20/10/2015 09:53, Qiu, Michael wrote: >> But as I know it is different all the time, am I right? >> If yes, I don't know what's the value of this field. > It can be used to get some snapshot/instant view informations while we > have to monitor and debug. So this field is mainly for debug? Thanks, Michael
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
On 2015/10/14 19:50, Ananyev, Konstantin wrote:
> Hi Amine,
>
>> -Original Message-
>> From: Amine Kherbouche [mailto:amine.kherbouche at 6wind.com]
>> Sent: Wednesday, October 14, 2015 12:40 PM
>> To: Ananyev, Konstantin; dev at dpdk.org
>> Subject: Re: [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX
>> queue information
>>
>>
>>
>> Hi Konstantin
>>> +/**
>>> + * Ethernet device RX queue information structure.
>>> + * Used to retieve information about configured queue.
>>> + */
>>> +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. */
>>> + uint16_t nb_desc; /**< configured number of RXDs. */
>> Here i need two more fields in this struct :
>> uint16_t free_desc : for free queue descriptors
>> uint16_t used_desc : for used queue descriptors
But as I know it is different all the time, am I right?
If yes, I don't know what's the value of this field.
Thanks,
Michael
>>> +} __rte_cache_aligned;
>
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
> > So your v6 will make all implemented queue_info_get()s to fill these two new > fields correctly, right? > Konstantin Yes. Amine Kherbouche
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
Hi Konstantin > Well, in general I am not opposed to add these fields to the structures. > In fact, I think it is a good thing to have. > But if we'll add them without making queue_info_get() feeling them properly - > it might create a lot of controversy. > So unless you prepared to make changes in all queue_info_get() too, > my opinion - let's leave it as it is for 2.2, and add new fields in later > releases. Thanks for your answer, I'm almost done with all changes in queue_info_get(), so i have just to test and send a series of v6 for your series. Best, Amine Kherbouche
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
Hi Konstantin
> +/**
> + * Ethernet device RX queue information structure.
> + * Used to retieve information about configured queue.
> + */
> +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. */
> + uint16_t nb_desc; /**< configured number of RXDs. */
Here i need two more fields in this struct :
uint16_t free_desc : for free queue descriptors
uint16_t used_desc : for used queue descriptors
> +} __rte_cache_aligned;
> +
> +/**
> + * Ethernet device TX queue information structure.
> + * Used to retieve information about configured queue.
> + */
> +struct rte_eth_txq_info {
> + struct rte_eth_txconf conf; /**< queue config parameters. */
> + uint16_t nb_desc; /**< configured number of TXDs. */
And also here.
> +} __rte_cache_aligned;
> +
> struct rte_eth_dev;
How to add them without breaking API ? I would prefer to see them now, so
I'll send an update on your patch series that i'll use this 2 more
fields. The purpose will be to provide analysis of the usage of the RX
and TX queues.
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
> -Original Message- > From: Amine Kherbouche [mailto:amine.kherbouche at 6wind.com] > Sent: Wednesday, October 14, 2015 1:47 PM > To: Ananyev, Konstantin; dev at dpdk.org > Subject: Re: [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX > queue information > > > > > > > So your v6 will make all implemented queue_info_get()s to fill these two > > new fields correctly, right? > > Konstantin > Yes. Ok great, will wait for v6 then. Konstantin > > Amine Kherbouche
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
> -Original Message- > From: Amine Kherbouche [mailto:amine.kherbouche at 6wind.com] > Sent: Wednesday, October 14, 2015 1:22 PM > To: Ananyev, Konstantin; dev at dpdk.org > Subject: Re: [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX > queue information > > > Hi Konstantin > > > Well, in general I am not opposed to add these fields to the structures. > > In fact, I think it is a good thing to have. > > But if we'll add them without making queue_info_get() feeling them properly > > - > > it might create a lot of controversy. > > So unless you prepared to make changes in all queue_info_get() too, > > my opinion - let's leave it as it is for 2.2, and add new fields in later > > releases. > Thanks for your answer, I'm almost done with all changes in > queue_info_get(), so i have just to > test and send a series of v6 for your series. So your v6 will make all implemented queue_info_get()s to fill these two new fields correctly, right? Konstantin > > Best, > Amine Kherbouche
[dpdk-dev] [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX queue information
Hi Amine,
> -Original Message-
> From: Amine Kherbouche [mailto:amine.kherbouche at 6wind.com]
> Sent: Wednesday, October 14, 2015 12:40 PM
> To: Ananyev, Konstantin; dev at dpdk.org
> Subject: Re: [dpdk-dev, PATCHv5, 1/8] ethdev: add new API to retrieve RX/TX
> queue information
>
>
>
> Hi Konstantin
> > +/**
> > + * Ethernet device RX queue information structure.
> > + * Used to retieve information about configured queue.
> > + */
> > +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. */
> > + uint16_t nb_desc; /**< configured number of RXDs. */
> Here i need two more fields in this struct :
> uint16_t free_desc : for free queue descriptors
> uint16_t used_desc : for used queue descriptors
> > +} __rte_cache_aligned;
Yep, that?s good thing to have.
But to fill them properly extra work is required, so my thought was to left it
for future expansion.
> > +
> > +/**
> > + * Ethernet device TX queue information structure.
> > + * Used to retieve information about configured queue.
> > + */
> > +struct rte_eth_txq_info {
> > + struct rte_eth_txconf conf; /**< queue config parameters. */
> > + uint16_t nb_desc; /**< configured number of TXDs. */
> And also here.
> > +} __rte_cache_aligned;
> > +
> > struct rte_eth_dev;
> How to add them without breaking API ?
As I said in the patch comments:
"left extra free space in the queue info structures,
so extra fields could be added later without ABI breakage."
As you can see both queue info structures have size 64B each.
So there is plenty of space for expansion without ABI breakage.
I would prefer to see them now, so
> I'll send an update on your patch series that i'll use this 2 more
> fields. The purpose will be to provide analysis of the usage of the RX
> and TX queues.
Well, in general I am not opposed to add these fields to the structures.
In fact, I think it is a good thing to have.
But if we'll add them without making queue_info_get() feeling them properly -
it might create a lot of controversy.
So unless you prepared to make changes in all queue_info_get() too,
my opinion - let's leave it as it is for 2.2, and add new fields in later
releases.
As I said above - ABI wouldn't be affected.
Konstantin
