> -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Wednesday, November 29, 2017 6:35 PM > To: Weglicki, MichalX <[email protected]> > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH] netdev: Custom statistics. > > On Tue, Nov 28, 2017 at 01:46:05PM +0000, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > Signed-off-by: Michal Weglicki <[email protected]> > > This is pretty cool. I like it. > > There is one new compiler warning: > > ../lib/string.c:49:1: error: no previous prototype for function > 'string_ends_with' [-Werror,-Wmissing-prototypes] [MW] I was quite confused how this happened and now I see that string.h is on git ignore list. I was trying to find good spot for string helper function, and didn't find anything more suitable. If this is on ignore list on purpose could you point me to some kind of string helper file where I can include my function? > > and new "sparse" warnings: > > ../lib/ofp-util.c:8100:40: warning: incorrect type in assignment > (different base types) > ../lib/ofp-util.c:8100:40: expected restricted ovs_be16 [usertype] > stats_array_size > ../lib/ofp-util.c:8100:40: got unsigned int const [unsigned] > [usertype] size > ../lib/ofp-util.c:8288:28: warning: incorrect type in assignment > (different base types) > ../lib/ofp-util.c:8288:28: expected unsigned int [unsigned] [usertype] > size > ../lib/ofp-util.c:8288:28: got restricted ovs_be16 const [usertype] > stats_array_size > ../lib/ofp-util.c:8297:34: warning: restricted ovs_be16 degrades to > integer [MW] Sorry about that, will do. > > It is inconvenient, in ofputil_append_ofp14_port_stats(), to make the > custom stats code know the size of the stats to be appended before > actually appending them. I think that it would lead to nicer code if we > dropped netdev_get_custom_stats_size(), etc., and just used > ofpmp_postappend() instead of ofpmp_reserve(). The latter is slightly > more efficient but it requires knowing the size in advance. Before this > patch that was easy, after this patch it is not, so a change in strategy > makes sense.
> > ofputil_append_ofp14_port_stats() and parse_intel_port_custom_property() > don't seem to be doing network byte order conversion for the counters. > > parse_intel_port_custom_property() doesn't seem to be careful not to > overrun the data buffer, or to make sure that the strings are of > reasonable lengths. > > Please add an item to NEWS. > > What's an appropriate place to document the new statistics? Is the set > of extended statistics fairly stable? If so, then maybe they could be > documented directly in OVS, otherwise I'd hope at least for a pointer to > wherever the real documentation is, from an appropriate place in the OVS > documentation. > > Thanks, > > Ben. Br, Michal. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
