[dpdk-dev] [PATCH] ethdev: check number of queues less than RTE_ETHDEV_QUEUE_STAT_CNTRS

[Alejandro Lucero]

Thomas,

We are wondering if you realize this patch fixes a bug with current ethdev
code as a device can have more than RTE_ETHDEV_QUEUE_STAT_CNTRS.

Maybe the commit message is giving the wrong impression and as you
commented, it should just focus on the bug it fixes and to leave for
another email thread the discussion of how to solve the
RTE_ETHDEV_QUEUE_STAT_CNTRS
problem.

Should we remove this from patchwork and to send another patch that way?


On Thu, Nov 10, 2016 at 4:04 PM, Alejandro Lucero <
alejandro.lucero at netronome.com> wrote:

>
>
> On Thu, Nov 10, 2016 at 4:01 PM, Thomas Monjalon <
> thomas.monjalon at 6wind.com> wrote:
>
>> 2016-11-10 15:43, Alejandro Lucero:
>> > On Thu, Nov 10, 2016 at 2:42 PM, Thomas Monjalon <
>> thomas.monjalon at 6wind.com>
>> > wrote:
>> >
>> > > 2016-11-10 14:00, Alejandro Lucero:
>> > > > From: Bert van Leeuwen <bert.vanleeuwen at netronome.com>
>> > > >
>> > > > A device can have more than RTE_ETHDEV_QUEUE_STAT_CNTRS queues which
>> > > > is used inside struct rte_eth_stats. Ideally, DPDK should be built
>> with
>> > > > RTE_ETHDEV_QUEUE_STAT_CNTRS to the maximum number of queues a device
>> > > > can support, 65536, as uint16_t is used for keeping those values for
>> > > > RX and TX. But of course, having such big arrays inside struct
>> > > rte_eth_stats
>> > > > is not a good idea.
>> > >
>> > > RTE_ETHDEV_QUEUE_STAT_CNTRS come from a limitation in Intel devices.
>> > > They have limited number of registers to store the stats per queue.
>> > >
>> > > > Current default value is 16, which could likely be changed to 32 or
>> 64
>> > > > without too much opposition. And maybe it would be a good idea to
>> modify
>> > > > struct rte_eth_stats for allowing dynamically allocated arrays and
>> maybe
>> > > > some extra fields for keeping the array sizes.
>> > >
>> > > Yes
>> > > and? what is your issue exactly? with which device?
>> > > Please explain the idea brought by your patch.
>> > >
>> >
>> > Netronome NFP devices support 128 queues and future version will support
>> > 1024.
>> >
>> > A particular VF, our PMD just supports VFs, could get as much as 128.
>> > Although that is not likely, that could be an option for some client.
>> >
>> > Clients want to use a DPDK coming with a distribution, so changing the
>> > RTE_ETHDEV_QUEUE_STAT_CNTRS depending on the present devices is not an
>> > option.
>> >
>> > We would be happy if RTE_ETHDEV_QUEUE_STAT_CNTRS could be set to 1024,
>> > covering current and future requirements for our cards, but maybe having
>> > such big arrays inside struct rte_eth_stats is something people do not
>> want
>> > to have.
>> >
>> > A solution could be to create such arrays dynamically based on the
>> device
>> > to get the stats from. For example, call to rte_eth_dev_configure could
>> > have ax extra field for allocating a rte_eth_stats struct, which will be
>> > based on nb_rx_q and nb_tx_q params already given to that function.
>> >
>> > Maybe the first thing to know is what people think about just
>> incrementing
>> > RTE_ETHDEV_QUEUE_STAT_CNTRS to 1024.
>> >
>> > So Thomas, what do you think about this?
>>
>> I think this patch is doing something else :)
>>
>>
> Sure. But the problem the patch solves is pointing to this, IMHO, bigger
> issue.
>
>
>> I'm not sure what is better between big arrays and variable size.
>> I think you must explain these 2 options in another thread,
>> because I'm not sure you will have enough attention in a thread starting
>> with
>> "check number of queues less than RTE_ETHDEV_QUEUE_STAT_CNTRS".
>>
>
> Agree. I'll do that then.
>
> Thanks
>