All of lore.kernel.org
 help / color / mirror / Atom feed
* xstats performance
@ 2016-06-29 15:38 Olivier MATZ
  2016-06-29 16:03 ` Remy Horton
  0 siblings, 1 reply; 8+ messages in thread
From: Olivier MATZ @ 2016-06-29 15:38 UTC (permalink / raw)
  To: dev, Remy Horton

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-06-29 15:38 xstats performance Olivier MATZ
@ 2016-06-29 16:03 ` Remy Horton
  2016-06-29 16:40   ` Thomas Monjalon
  0 siblings, 1 reply; 8+ messages in thread
From: Remy Horton @ 2016-06-29 16:03 UTC (permalink / raw)
  To: Olivier MATZ, dev

'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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-06-29 16:03 ` Remy Horton
@ 2016-06-29 16:40   ` Thomas Monjalon
  2016-07-01  9:15     ` Remy Horton
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2016-06-29 16:40 UTC (permalink / raw)
  To: Remy Horton; +Cc: dev, Olivier MATZ

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-06-29 16:40   ` Thomas Monjalon
@ 2016-07-01  9:15     ` Remy Horton
  2016-07-05 18:10       ` Rasesh Mody
  0 siblings, 1 reply; 8+ messages in thread
From: Remy Horton @ 2016-07-01  9:15 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Olivier MATZ


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-07-01  9:15     ` Remy Horton
@ 2016-07-05 18:10       ` Rasesh Mody
  2016-07-06  7:43         ` Remy Horton
  0 siblings, 1 reply; 8+ messages in thread
From: Rasesh Mody @ 2016-07-05 18:10 UTC (permalink / raw)
  To: Remy Horton, Thomas Monjalon; +Cc: dev, Olivier MATZ

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-07-05 18:10       ` Rasesh Mody
@ 2016-07-06  7:43         ` Remy Horton
  2016-07-07 22:59           ` Rasesh Mody
  0 siblings, 1 reply; 8+ messages in thread
From: Remy Horton @ 2016-07-06  7:43 UTC (permalink / raw)
  To: Rasesh Mody, Thomas Monjalon; +Cc: dev, Olivier MATZ


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.. :)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-07-06  7:43         ` Remy Horton
@ 2016-07-07 22:59           ` Rasesh Mody
  2016-07-08 11:15             ` Remy Horton
  0 siblings, 1 reply; 8+ messages in thread
From: Rasesh Mody @ 2016-07-07 22:59 UTC (permalink / raw)
  To: Remy Horton, Thomas Monjalon; +Cc: dev, Olivier MATZ

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: xstats performance
  2016-07-07 22:59           ` Rasesh Mody
@ 2016-07-08 11:15             ` Remy Horton
  0 siblings, 0 replies; 8+ messages in thread
From: Remy Horton @ 2016-07-08 11:15 UTC (permalink / raw)
  To: Rasesh Mody, Thomas Monjalon; +Cc: dev, Olivier MATZ


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-07-08 11:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-29 15:38 xstats performance Olivier MATZ
2016-06-29 16:03 ` Remy Horton
2016-06-29 16:40   ` Thomas Monjalon
2016-07-01  9:15     ` Remy Horton
2016-07-05 18:10       ` Rasesh Mody
2016-07-06  7:43         ` Remy Horton
2016-07-07 22:59           ` Rasesh Mody
2016-07-08 11:15             ` Remy Horton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.