Re: [PATCH v2] ethdev: check number of queues less than RTE_ETHDEV_QUEUE_STAT_CNTRS

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-11-24 17:59, Olivier Matz:
> Hi,
> 
> On Mon, 2016-11-21 at 09:59 +0000, Alejandro Lucero wrote:
> > From: Bert van Leeuwen <bert.vanleeuwen@netronome.com>
> > 
> > Arrays inside rte_eth_stats have size=RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > Some devices report more queues than that and this code blindly uses
> > the reported number of queues by the device to fill those arrays up.
> > This patch fixes the problem using MIN between the reported number of
> > queues and RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > 
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > 
> 
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
> 
> 
> As a next step, I'm wondering if it would be possible to remove
> this limitation. We could replace the tables in struct rte_eth_stats
> by a pointer to an array allocated dynamically at pmd setup.

Yes that's definitely the right way to handle these statistics.

> It would break the API, so it should be announced first. I'm thinking
> of something like:
> 
> struct rte_eth_generic_stats {
>         uint64_t ipackets;
>         uint64_t opackets;
>         uint64_t ibytes;
>         uint64_t obytes;
>         uint64_t imissed;
>         uint64_t ierrors;
>         uint64_t oerrors;
>         uint64_t rx_nombuf
> };
> 
> struct rte_eth_stats {
> 	struct rte_eth_generic_stats port_stats;
> 	struct rte_eth_generic_stats *queue_stats;
> };
> 
> The queue_stats array would always be indexed by queue_id.
> The xstats would continue to report the generic stats per-port and
> per-queue.
> 
> About the mapping API, either we keep it as-is, or it could
> become a driver-specific API.

Yes I agree to remove the queue statistics mapping which is very specific.
I will send a patch with a deprecation notice to move the mapping API
to a driver-specific API.

Any objection?