On Wed, Feb 28, 2018 at 08:05:56PM +0530, SatyaValli wrote: > From: SatyaValli <satyavalli.r...@tcs.com> > > This Patch provides implementation Existing flow entry statistics are > redefined as standard OXS(OpenFlow Extensible Statistics) fields for > displaying the arbitrary flow stats.The existing Flow Stats were renamed > as Flow Description.
Thanks for working on this. I have some comments. A big issue here is naming. OF1.5 renamed "flow statistics" to "flow description" and introduced something new named "flow statistics" that is different from the previous meaning. This is really confusing. In the past, when this sort of renaming has happened, we've usually taken one of two different approaches: 1. Use the old name everywhere and invent a new name for whatever OF newly invented. For example, in this case we would continue to use "flow statistics" for its old pre-OF1.5 meaning and invent some new name for OF1.5 flow statistics (from the OF1.5 release notes I suggest "lightweight flow statistics"). 2. Rename everything in the tree to match the new names. For example, in this case we would rename everything involved with flow statistics, regardless of OpenFlow version, to flow description, and then use the new names. We generally find that using a mix of names according to OpenFlow version is more confusing than either of these choices. In this case, I recommend choice #1 above, since the name "flow statistics" is pretty ingrained in the OVS source code and among OpenFlow and OVS users. It's going to be less confusing. So I'd go through and replace OF1.5 FLOW_STATS by something new like LIGHTWEIGHT_FLOW_STATS (that's pretty long so maybe some shorter name can be invented) and then replace FLOW_DESC by FLOW_STATS. Overall, it is likely to be less confusing. There is another way that the approach here is a little different from the ones that we normally take. Usually, when some new version of OpenFlow offers increased granularity of some functionality, or a new feature on top of functionality, we make an attempt to provide some uniformity in the interface, to the extent that we can. For example, existing OpenFlow versions already provide all of the kinds of statistics that the predefined OXS constants provide. It is possible therefore, at some level, to simply pretend that previous OpenFlow versions have OXS support, just that the OXS fields that they support is fixed. In this viewpoint, for example, one might make ofp_print_flow_stats() take an 'oxs' argument instead of a 'show_stats' (or in addition, since the latter controls more than what OXS considers statistics) argument and just print the requested statistics. Usually this "more uniform" view of OpenFlow works out better in the long term. It looks to me that ofp15_flow_stats_request is identical to ofp11_flow_stats_request. If so, please do not define it at all: use the existing structure. I think the new block in ofputil_encode_flow_stats_request() is identical to the existing OXM block except for the raw value in the non-aggregate case. Please avoid so much cut-and-paste for such a trivial change. There is some funny indentation in ofputil_decode_flow_stats_reply(). Please adjust it. ofputil_decode_flow_removed() must honor the error return value from oxs_flow_removed_stat_pull(). The only change to ofp-parse.c is a new #include, which is probably unneeded. I don't understand why ofp_print_flow_oxs_stats() takes its 'oxs' argument and does nothing if 'oxs' is true. Why call the function at all in that case? And in fact the only caller appears to never call it with oxs == true. Same question for ofp_print_flow_oxs_agg_stats(). The oxs_bitmap_*() functions don't look to me like they provide a useful abstraction. I would drop them. I think that this patch is making a lot of progress and I'll look forward to the next version. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev