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

Reply via email to