[ovs-dev] [PATCH] OF1.5/EXT-334 OXS/Extensible Flow Entry Statistics Support
Satyavalli Rama
satyavalli.rama at tcs.com
Fri Aug 4 13:19:40 UTC 2017
Hi Ben,
Much Thanks for the Review.
We will address the comments and will submit the updated patch soon.
Thanks & Regards
Satya Valli
Tata Consultancy Services
Mailto: satyavalli.rama at tcs.com
Website: http://www.tcs.com
____________________________________________
Experience certainty. IT Services
Business Solutions
Consulting
____________________________________________
From: Ben Pfaff <blp at ovn.org>
To: SatyaValli <satyavalli.rama at gmail.com>
Cc: dev at openvswitch.org, Surya Muttamsetty
<muttamsetty.surya at tcs.com>, SatyaValli <satyavalli.rama at tcs.com>
Date: 08/03/2017 01:41 AM
Subject: Re: [ovs-dev] [PATCH] OF1.5/EXT-334 OXS/Extensible Flow
Entry Statistics Support
Thanks for the revision. I have some more comments.
On Mon, Jun 19, 2017 at 01:14:40PM +0530, SatyaValli wrote:
> commit 49a3592b2878a33033e5fd2e6e5ce82ebccef780
> Author: Satya Valli <satyavalli.rama at tcs.com>
> Co-authored-by: Lavanya Harivelam <harivelam.lavanya at tcs.com>
> Co-authored-by: Surya Muttamsetty <muttamsetty.surya at tcs.com>
None of the above should be at the top of the commit message. Coauthors
go with the sign-offs at the end.
> OpenVswitch: OF1.5/EXT-334 Extensible Flow Entry Statistics
Please don't repeat the subject again in the body.
> OpenFlow 1.5 introduces the Extensible Statistics (OXS) by redefining
the existing
> flow entry statistics with OXS Fields.
> This Patch provides implementation for OXS fields encoding in TLV
format.
>
> To support this implementation below two messages are newly added
>
> OFPST_OXS_FLOW_STATS_REQUEST
> OFPST_OXS_FLOW_STATS_REPLY
> OFPST_OXS_AGGREGATE_STATS_REQUEST
> OFPST_OXS_AGGREGATE_STATS_REPLY
> OFPST_FLOW_REMOVED
> As per the openflow specification-1.5, this enhancement should take
place
> on the existing flow entry statistics with the OXS fields on all the
messages
> that carries flow entry statistics.
>
> The current commit adds support for the new feature in flow statistics
multipart messages,
> aggregate multipart messages and OXS flow statistics support for flow
removal message.
>
> Some more fields are added to ovs-ofctl dump-flows command to support
OpenFlow15 OXS stats.
> Below are Commands to display OXS stats field wise
>
> Flow Statistics Multipart
> ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-duration
> ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-idle_time
> ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-packet_count
> ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-byte_count
>
> Aggregate Flow Statistics Multipart
> ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-packet_count
> ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-byte_count
> ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-flow_count
>
> Make Check Changes in - ofproto.at and ofproto-dpif.at:
> For 1.5 version "flags" variable has been removed from flow_stats_reply
structure.
> For avoiding the make check failures for OF1.5 version add-flow with
flags,
> below test cases has been modified for 1.5 version in respective .at
files
>
> Files testcase
> ofproto.at 0884
> ofproto-dpif 1126
Please don't refer to tests by number. The numbers are not meaningful
and change often. Use the name of the test.
> In ofproto.at there is one test case numbered 0884 and 1126 in
ofproto-dpif.at
> which is validating flagsfor OF1.5 this testcase is not valid, because
> in stats reply flags are not explicitly communicated,I've removed this
test case.
> Before removing this test case is failing for 1.5 version.
>
> Since FLOW_REMOVED is not supported in OF1.5 version,
> testing has been done by adding test case in ofproto.at.
> While doing make check after adding test case we are able to see
> OFPT_FLOW_REMOVED (OF1.5) message has sent successfully.
>
> But make check is failing because of Signal 15 termination,
> so we've removed that test case from the patch.
Why isn't FLOW_REMOVED supported in OF1.5? Support is needed. Signal
15 is SIGTERM, meaning that something actually used "kill" to kill it
intentionally. Please investigate and fix the problem rather than
removing a test.
This adds features that users can use via ovs-ofctl, but it does not
document them. Please document them in ovs-ofctl(8). Please also add
appropriate NEWS items to explain the new features.
This adds support for new OpenFlow messages, but it does not add any
tests for printing them in ofp-print.at. Please add at least one
representative test there for every new message.
struct ofp15_flow_removed doesn't seem to be the same as
ofp_flow_removed in OpenFlow 1.5.
This adds a global variable oxs_field_set. This is inappropriate. Do
not use global variables.
The variable oxs_field_set has a bunch of unnamed magic numbers. This
is inappropriate. Do not use unnamed magic numbers.
This code is weird. Fix it please:
+ if (parse_oxs_field(name, &f)) {
+ if (f->fl_type == OFPXST_OFB_DURATION) {
+ oxs_field_set |= (1 << f->fl_type);
+ } else if (f->fl_type == OFPXST_OFB_IDLE_TIME) {
+ oxs_field_set |= (1 << f->fl_type);
+ } else if (f->fl_type == OFPXST_OFB_FLOW_COUNT) {
+ oxs_field_set |= (1 << f->fl_type);
+ } else if (f->fl_type == OFPXST_OFB_PACKET_COUNT) {
+ oxs_field_set |= (1 << f->fl_type);
+ } else if (f->fl_type == OFPXST_OFB_BYTE_COUNT) {
+ oxs_field_set |= (1 << f->fl_type);
+ }
This patch seems not to recognize that OF1.5+ has two different ways to
request and get replies for flow stats: FLOW_DESC and FLOW_STATS. It
looks like it only supports the latter. We must support both.
Thanks,
Ben.
=====-----=====-----=====
Notice: The information contained in this e-mail
message and/or attachments to it may contain
confidential or privileged information. If you are
not the intended recipient, any dissemination, use,
review, distribution, printing or copying of the
information contained in this e-mail message
and/or attachments to it are strictly prohibited. If
you have received this communication in error,
please notify us by reply e-mail or telephone and
immediately and permanently delete the message
and any attachments. Thank you
More information about the dev
mailing list