xstats performance

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

'noon,

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

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

2016-06-29 17:03, Remy Horton:

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.

On 29/06/2016 17:40, Thomas Monjalon wrote:
[..]

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

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

Hi Remy,

We could incorporate this change in our re-submission.

On 05/07/2016 19:10, Rasesh Mody wrote:
[..]

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

On 07/07/2016 23:59, Rasesh Mody wrote:
[..]

Seen & Acked.

Had a look. Patch on the way..

..Remy