[dpdk-dev] xstats performance

[dpdk-dev] xstats performance

[Olivier MATZ]

Hi Remy,

While adapting an application to the new xstats API, I discovered
that it may not be so efficient to display the statistics and their
names.

I think the test-pmd code illustrates the issue pretty well:

/* Display xstats */
for (idx_xstat = 0; idx_xstat < cnt_xstats; idx_xstat++)
	for (idx_name = 0; idx_name < cnt_xstats; idx_name++)
		if (xstats_names[idx_name].id == xstats[idx_xstat].id) {
			printf("%s: %"PRIu64"\n",
				xstats_names[idx_name].name,
				xstats[idx_xstat].value);
			break;
		}

The displaying is in O(n^2).

It's possible to enhance the code to have it in O(n), but it
requires an intermediate table.

Why not changing this:

   struct rte_eth_xstat {
   	uint64_t id;
   	uint64_t value;
   };
   struct rte_eth_xstat_name {
   	char name[RTE_ETH_XSTATS_NAME_SIZE];
   	uint64_t id;
   };

Into this:

   struct rte_eth_xstat {
   	uint64_t id;
   	uint64_t value;
   };
   struct rte_eth_xstat_name {
   	char name[RTE_ETH_XSTATS_NAME_SIZE];
   	/* No identifier */
   };

And assume that the id field in rte_eth_xstat corresponds to
the index in the rte_eth_xstat_name table?


The test-pmd code would be something like this:

/* Display xstats */
for (idx_xstat = 0; idx_xstat < cnt_xstats; idx_xstat++) {
	printf("%s: %"PRIu64"\n",
		xstats_names[xstats[idx_xstats].id].name,
		xstats[idx_xstat].value);
}


What do you think?

Regards
Olivier

Re: [dpdk-dev] xstats performance

[Remy Horton]

'noon,

On 29/06/2016 16:38, Olivier MATZ wrote:
[..]

> And assume that the id field in rte_eth_xstat corresponds to
> the index in the rte_eth_xstat_name table?

It was an assumption I wanted to avoid setting in stone, even though at 
the moment it is actually true in implementation.

The mappings are driver-specific but I wanted to leave open, amoung 
other things, the possibility of string-id mappings being standardised 
across all drivers. For that to work drivers would have to be able to 
use a set of ids that may contain gaps.

..Remy

Re: [dpdk-dev] xstats performance

[Thomas Monjalon]

2016-06-29 17:03, Remy Horton:
> On 29/06/2016 16:38, Olivier MATZ wrote:
> > And assume that the id field in rte_eth_xstat corresponds to
> > the index in the rte_eth_xstat_name table?
> 
> It was an assumption I wanted to avoid setting in stone, even though at 
> the moment it is actually true in implementation.
> 
> The mappings are driver-specific but I wanted to leave open, amoung 
> other things, the possibility of string-id mappings being standardised 
> across all drivers. For that to work drivers would have to be able to 
> use a set of ids that may contain gaps.

I don't think it is possible to standardize stats ids, for two reasons:
- it is hard to maintain and avoid conflicts between drivers
- the drivers would have to lookup the names which degrades performance

I think the idea of Olivier would improve the performance of stats retrieval,
which was the idea of this rework :)
Unfortunately we need someone available to fix it quickly for RC2.

Re: [dpdk-dev] xstats performance

[Remy Horton]

On 29/06/2016 17:40, Thomas Monjalon wrote:
[..]
> I don't think it is possible to standardize stats ids, for two reasons:
> - it is hard to maintain and avoid conflicts between drivers
> - the drivers would have to lookup the names which degrades performance

I designed it that way to keep flexibility down the line rather than 
specifically for the above use-case.


> I think the idea of Olivier would improve the performance of stats retrieval,
> which was the idea of this rework :)
> Unfortunately we need someone available to fix it quickly for RC2.

For all the current drivers xstats_names[idx].id==idx so it would just 
involve removing the references to the id field and updating the 
documentation. Complication is coordinating with QLogic for the bnx2x & 
qede xstats patches.

..Remy

Re: [dpdk-dev] xstats performance

[Rasesh Mody]

Hi Remy,

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, July 01, 2016 2:15 AM
> 
> 
> On 29/06/2016 17:40, Thomas Monjalon wrote:
> [..]
> > I don't think it is possible to standardize stats ids, for two reasons:
> > - it is hard to maintain and avoid conflicts between drivers
> > - the drivers would have to lookup the names which degrades
> > performance
> 
> I designed it that way to keep flexibility down the line rather than specifically
> for the above use-case.
> 
> 
> > I think the idea of Olivier would improve the performance of stats retrieval,
> > which was the idea of this rework :)
> > Unfortunately we need someone available to fix it quickly for RC2.
> 
> For all the current drivers xstats_names[idx].id==idx so it would just
> involve removing the references to the id field and updating the
> documentation. Complication is coordinating with QLogic for the bnx2x &
> qede xstats patches.

We could incorporate this change in our re-submission.
 
> ..Remy

Re: [dpdk-dev] xstats performance

[Remy Horton]

On 05/07/2016 19:10, Rasesh Mody wrote:
[..]
>> For all the current drivers xstats_names[idx].id==idx so it would just
>> involve removing the references to the id field and updating the
>> documentation. Complication is coordinating with QLogic for the bnx2x &
>> qede xstats patches.
>
> We could incorporate this change in our re-submission.

The changes mentioned above have been merged in, so go ahead.. :)

Re: [dpdk-dev] xstats performance

[Rasesh Mody]

> From: Remy Horton [mailto:remy.horton@intel.com]
> Sent: Wednesday, July 06, 2016 12:44 AM
> 
> 
> On 05/07/2016 19:10, Rasesh Mody wrote:
> [..]
> >> For all the current drivers xstats_names[idx].id==idx so it would
> >> just involve removing the references to the id field and updating the
> >> documentation. Complication is coordinating with QLogic for the bnx2x
> >> & qede xstats patches.
> >
> > We could incorporate this change in our re-submission.
> 
> The changes mentioned above have been merged in, so go ahead.. :)

We have submitted the QEDE and BNX2X PMD xstats patches with recent API changes.

During the xstats change verification, it was observed that the order in which xstats names are fetched don't seem to match the order in which xstats values are fetched. When populating the xstats names, we order them as driver specific xstats names and then standard stats names. Whereas, while populating the xstats values, we populate the standard stats values and then the driver specific xstats values.

In rte_eth_xstats_get_names(): xcount (or cnt_used_entries) + cnt_used_entries/count (RTE_NB_STATS + (dev->data->nb_rx_queues * RTE_NB_RXQ_STATS) + (dev->data->nb_tx_queues * RTE_NB_TXQ_STATS))
In rte_eth_xstats_get (): count (RTE_NB_STATS + (dev->data->nb_rx_queues * RTE_NB_RXQ_STATS) + (dev->data->nb_tx_queues * RTE_NB_TXQ_STATS)) + xcount

Could you take a look?

Thanks!
Rasesh

Re: [dpdk-dev] xstats performance

[Remy Horton]

On 07/07/2016 23:59, Rasesh Mody wrote:
[..]
> We have submitted the QEDE and BNX2X PMD xstats patches with recent
> API changes.

Seen & Acked.


> During the xstats change verification, it was observed that the order
> in which xstats names are fetched don't seem to match the order in
> which xstats values are fetched. When populating the xstats names, we
> order them as driver specific xstats names and then standard stats
> names. Whereas, while populating the xstats values, we populate the
> standard stats values and then the driver specific xstats values.

Had a look. Patch on the way..

..Remy