> > > 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?
>
>
> > 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-
> >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.
>
>
> > 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.
>
>
> > 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.
>
> > 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.
>
>
> > 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.
>
>
> > > 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
