Re: Future Direction for rte_eth_stats_get()

Hi folks,

I didn't see any follow up to this response.

My original concern was rte_eth_stats_get() moving away from a more conventional based definition (note, I believe Matthew Hall made an interesting suggestion to follow a MIB based definition elsewhere).  However, if modifying that API is not desired then I'd really like to have some feedback about extending the current xstats model.

Again, it is desired not to have to copy and/or parse strings for scalability reasons but still maintain the "ABI flexibility" for which the xstats model was geared towards.

Thanks,
Dave

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton (dharton)
Sent: Friday, January 22, 2016 2:26 PM
To: Van Haaren, Harry <redacted>; Thomas Monjalon <redacted>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

quoted

quoted

quoted

Do you see a fundamental problem with parsing the strings to
retrieve values?

I think parsing strings is expensive CPU wise

Parsing the strings can be done once at startup, and the index of the
statistics in the array cached. When actually reading statistics,
access of the array of statistics at the previously cached index will
return the same statistic. It is not needed to do strcmp() per statistic

per read.

How is this order guaranteed through the API?  The stat is currently
identified by the string not by order returned.  What if a driver returns
more/less stats based on config that changes after boot?

quoted

quoted

That's why I was wondering if a binary id could be generated instead.

I've worked with such a system before, it dynamically registered
string-

quoted

int mappings at runtime, and allowed "reverse" mapping them too. Its

workable, and I'm not opposed to it if the conclusion is that it
really is necessary, but I'm not sure about that.

quoted

The API
would be very similar to the current xstats API except it would pass
id/value pairs instead of string/value pairs.  This avoids string
comparisons to figure out while still allowing flexibility between

drivers.

Apart from a once-off scan at startup, xstats achieves this.

Partially true.  You may not need to perform strcmp() per read, although I
don't believe the defined API guarantees that this would be safe (see
above); but, you still have to perform a strcpy() of each stat whether the
caller is interested in it or not.

quoted

quoted

 It would also 100% guarantee that any strings returned by "const
char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent
across devices.

Yes - that is a nice feature that xstats (in its current form) doesn't
have.
But what price is there to pay for this?
We need to map an ID for each stat that exists.

How and where will this mapping happen? Perhaps would you expand a
little on how you see this working?

I wasn't thinking dynamic registration as that might be more than
necessary.  I can code up a detailed snippet that shares the idea if
wanted but I think the following communicates the basic idea:

enum rte_eth_stat_e {
    /* accurate desc #1 */
    RTE_ETH_STAT_1,
    /* accurate desc #2 */
    RTE_ETH_STAT_2,
...
}
struct rte_eth_id_stat {
    rte_eth_stat_e id;
    uin64_t value;
}

int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
/* returns < 0 on error or the number of stats that could have been read
(i.e. if userd  */ int rte_eth_id_stats_get(uint8_t port_id, uint32_t
num_stats, rte_eth_id_stat *id_stats); const char*
rte_eth_id_stat_str(rte_eth_stat_e id);

This allows a driver to return whatever stats that it supports in a
consistent manner and also in a performance friendly way.  In fact, the
driver side would be identical to what they have today but instead of
having arrays with "string stat name" they will have the rte_eth_stat_e.

quoted

quoted

I also think there is less chance for error if drivers assign their
stats to ID versus constructing a string.

Have a look in how the mapping is done in the xstats implementation
for ixgbe for example:
http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540

It's a pretty simple  {"string stat name" -> offsetof( stuct hw,
register_var_name )}

It's not how they add the strings but rather the format of the string
itself that is error prone.
IOTW, the "string stat name" is prone to not being formatted correctly or
consistently.

quoted

quoted

I haven't checked my application, but I wonder if the binary size
would actually be smaller if all the stats strings were centrally
located versus distributed across binaries (highly dependent on the

linker and optimization level).

Sounds like optimizing the wrong thing to me.

Wasn't trying to optimize image size as much as saying it's a side benefit
due to string consolidation.

quoted

quoted

quoted

If you like I could code up a minimal sample-app that only pulls
statistics, and you can show "interest" in various statistics for
printing / monitoring?

Appreciate the offer.  I actually understand what's is available.
And, BTW, I apologize for being late to the game (looks like this
was discussed last summer) but I'm just now getting looped in and
following the mailer but I'm just wondering if something that is

performance friendly for the user/application and flexible for the
drivers is possible.

We're all looking for the same thing - just different approaches
that's all :)


RE: Thomas asking about performance numbers:
I can scrape together some raw tsc data on Monday and post to list,
and we can discuss it more then.

I can do the same if desired.  But, just to make sure we are discussing
the same issue:

1) call rte_eth_xtats_get()
This will result in many string copies and depending on the driver *many*
copies I don't want or care about.
2) "tokenize"/parse/hash the string returned to identify what the stat
actually is I'm guessing you are stating that this step could be mitigated
at startup.  But, again, I don't think the API provides a guarantee which
usually leads to bugs over time.
3) Copy the value of the stat into the driver agnostic container the
application uses
4) Repeat steps 1-3 for every interface being serviced every 5 or 10 secs

Contrast that to my suggestion which has no string copies and a compile
time mapping between "stat_id" and "app stat" can be created for step 2.
I think the performance differences are obvious even without generating
cycle times.

I know that dpdk is largely focused on data plane performance and rightly
so, but, I'm hoping we can keep the control plane in mind as well
especially in the areas around stats or things that happen periodically
per port.

Regards,
Dave