[dpdk-dev] [PATCH v2 02/15] eventdev: add APIs for extended stats

[Jerin Jacob]

On Mon, Feb 06, 2017 at 10:37:31AM +0000, Van Haaren, Harry wrote:
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob at caviumnetworks.com]
> > Sent: Monday, February 6, 2017 8:23 AM
> > To: Van Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>
> > Subject: Re: [PATCH v2 02/15] eventdev: add APIs for extended stats
> > 
> > On Tue, Jan 31, 2017 at 04:14:20PM +0000, Harry van Haaren wrote:
> > > From: Bruce Richardson <bruce.richardson at intel.com>
> > >
> > > Add in APIs for extended stats so that eventdev implementations can report
> > > out information on their internal state. The APIs are based on, but not
> > > identical to, the equivalent ethdev functions.
> > 
> > The APIs Looks good. One minor comment though,

One more suggestion,

How about adding the reset function for resetting the selective xstat counters similar to
ethdev? IMO, It will be useful.

> > 
> > Can you add statistics specific to per event queue and event
> > port?, To improve the cases like below in the application code(taken from
> > app/test/test_sw_eventdev.c).
> > 
> > IMO, it is useful because,
> > - ethdev has similar semantics
> > - majority of the implementations will have port and queue specific statistics counters
> 
> I'm not totally sure what you're asking but if I understand correctly, you're suggesting a struct based stats API like this?
> 
> struct rte_event_dev_port_stats {
>     uint64_t rx;
>     uint64_t tx;
>     ...
> };
> 
> struct rte_event_dev_queue_stats {
>     uint64_t rx;
>     uint64_t tx;
>     ...
> };
> 
> /** a get function to get a specific port's statistics. The *stats* pointer is filled in */ 
> int rte_event_dev_port_stats_get(dev, uint8_t port_id, struct rte_event_dev_port_stats *stats);
> 
> /** a get function to get a specific queue's statistics. The *stats* pointer is filled in */ 
> int rte_event_dev_queue_stats_get(dev, uint8_t queue_id, struct rte_event_dev_queue_stats *stats);
> 
> 
> Is this what you meant, or did I misunderstand?

I meant, queue and port specific "xstat" as each implementation may
have different statistics counters for queue/port.

Just to share my view, I have modified the exiting proposal.
Thoughts?

+enum rte_event_dev_xstats_mode {
+       RTE_EVENT_DEV_XSTAT_DEVICE; /* Event device specific global xstats */
+       RTE_EVENT_DEV_XSTAT_QUEUE; /* Event queue specific xstats */
+       RTE_EVENT_DEV_XSTAT_PORT; /* Event port specific xstats */
+};
+
 /**
  * Retrieve names of extended statistics of an event device.
  *
@@ -1436,9 +1442,11 @@ struct rte_event_dev_xstat_name {
  */
 int
 rte_event_dev_xstats_names_get(uint8_t dev_id,
+                              enum rte_event_dev_xstats_mode mode;
                               struct rte_event_dev_xstat_name *xstat_names,
                               unsigned int size);

 /**
  * Retrieve extended statistics of an event device.
  *
@@ -1458,7 +1466,10 @@ rte_event_dev_xstats_names_get(uint8_t dev_id,
  *     device doesn't support this function.
  */
 int
-rte_event_dev_xstats_get(uint8_t dev_id, const unsigned int ids[],
+rte_event_dev_xstats_get(uint8_t dev_id,
+                        enum rte_event_dev_xstats_mode mode,
+                        uint8_t queue_port_id; /* valid when RTE_EVENT_DEV_XSTAT_QUEUE or RTE_EVENT_DEV_XSTAT_PORT */
+                        const unsigned int ids[],
                         uint64_t values[], unsigned int n);
 
 /**
@@ -1478,7 +1489,9 @@ rte_event_dev_xstats_get(uint8_t dev_id, const
unsigned int ids[],
  *   - negative value: -EINVAL if stat not found, -ENOTSUP if not
  *   supported.
  */
 uint64_t
-rte_event_dev_xstats_by_name_get(uint8_t dev_id, const char *name,
+rte_event_dev_xstats_by_name_get(uint8_t dev_id,
+                                enum rte_event_dev_xstats_mode mode,
+                                const char *name,
                                 unsigned int *id);
> 
> 
> > 
> > +       for (i = 0; i < MAX_PORTS; i++) {
> > +               char name[32];
> > +               snprintf(name, sizeof(name), "port_%u_rx", i);
> > +               stats->port_rx_pkts[i] = rte_event_dev_xstats_by_name_get(
> > +                               dev_id, name, &port_rx_pkts_ids[i]);
> > 
> > +       for (i = 0; i < MAX_QIDS; i++) {
> > +               char name[32];
> > +               snprintf(name, sizeof(name), "qid_%u_rx", i);
> > +               stats->qid_rx_pkts[i] = rte_event_dev_xstats_by_name_get(
> > +                               dev_id, name, &qid_rx_pkts_ids[i]);
> > 
>