Hi Aaron Please ignore the current patch. Due to some proxy issues we were unable to send the complete patches. Soon, we wil submitt fresh patches.
Thanks & Regards Satya Valli Tata Consultancy Services Mailto: satyavalli.r...@tcs.com Website: http://www.tcs.com ____________________________________________ Experience certainty. IT Services Business Solutions Consulting ____________________________________________ -----Aaron Conole <acon...@redhat.com> wrote: ----- To: Ben Pfaff <b...@ovn.org>, Satya Valli <satyavalli.r...@tcs.com> From: Aaron Conole <acon...@redhat.com> Date: 05/11/2017 11:23PM Cc: d...@openvswitch.org, Subject: Re: [ovs-dev] [PATCH 1/2] OF1.5/EXT-334 OXS/Individal Flow Statistics Hi Satya, I haven't checked the OF1.5 spec with this, yet. Just saw a few things. Thanks for the contribution! Ben Pfaff <b...@ovn.org> writes: > From: Satya Valli <satyavalli.r...@tcs.com> > Missing signed-off-by line. Also, it would be good to describe exactly which flow stat types are being provided. > --- > include/openflow/openflow-1.5.h | 48 > +++++++++++++++++++++++++++++++++++++++++ > include/openvswitch/ofp-msgs.h | 6 ++++++ > lib/automake.mk | 2 ++ > lib/ofp-parse.c | 45 +++++++++++++++++++++++++++++++++++++- > lib/ofp-print.c | 10 +++++++++ > lib/rconn.c | 2 ++ > ofproto/ofproto.c | 4 ++++ > 7 files changed, 116 insertions(+), 1 deletion(-) > > diff --git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h > index 3649e6c29e63..ff5fa13fae4a 100644 > --- a/include/openflow/openflow-1.5.h > +++ b/include/openflow/openflow-1.5.h > @@ -150,4 +150,52 @@ struct ofp15_group_desc_stats { > }; > OFP_ASSERT(sizeof(struct ofp15_group_desc_stats) == 16); > > +struct ofp_oxs_stat { > + ovs_be16 reserved; /* One of OFPST_* */ > + ovs_be16 length; /* Stats Length */ > +}; > +OFP_ASSERT(sizeof(struct ofp_oxs_stat) == 4); > + > +/*Body for ofp_multipart_request of type > + OFPMP_FLOW_DESC & OFPMP_FLOW_STATS.*/ > +struct ofp15_oxs_flow_stats_request { > + uint8_t table_id; /* ID of table to read > + (from ofp_table_desc), > + OFPTT_ALL for all tables. */ > + uint8_t pad[3]; /* Align to 32 bits. */ > + ovs_be32 out_port; /* Require matching entries to include > + this as an output port. A value of > + OFP_ANY indicates no restriction. */ > + ovs_be32 out_group; /* Require matching entries to include > + this as an output group. A value of > + OFPG_ANY indicates no restriction. > */ > + uint8_t pad2[4]; /* Align to 64 bits. */ > + ovs_be64 cookie; /* Require matching entries to contain > + this cookie value */ > + ovs_be64 cookie_mask; /* Mask used to restrict the cookie > bits > + that must match. A value of 0 > + indicates no restriction. */ > +}; > +OFP_ASSERT(sizeof(struct ofp15_oxs_flow_stats_request) == 32); > + > +/* Body of reply to OFPMP_FLOW_STATS request > +* and body for OFPIT_STAT_TRIGGER generated status. */ Minor nit - please put a space at the beginning of the comment line here. > +struct ofp15_oxs_flow_stats_reply { > + ovs_be16 length; /* Length of this entry. */ > + uint8_t pad2[2]; /* Align to 64-bits. */ > + uint8_t table_id; /* ID of table flow came from. */ > + uint8_t reason; /* One of OFPFSR_*. */ > + ovs_be16 priority; /* Priority of the entry. */ > +}; > +OFP_ASSERT(sizeof(struct ofp15_oxs_flow_stats_reply) == 8); > + > +/* OXS flow stat field types for OpenFlow basic class. */ > +enum oxs_ofb_stat_fields { > + OFPXST_OFB_DURATION = 0, /* Time flow entry has been alive. */ > + OFPXST_OFB_IDLE_TIME = 1, /* Time flow entry has been idle. */ > + OFPXST_OFB_FLOW_COUNT = 2, /* Number of aggregated flow entries. */ > + OFPXST_OFB_PACKET_COUNT = 3, /* Number of packets in flow entry. */ > + OFPXST_OFB_BYTE_COUNT = 4, /* Number of bytes in flow entry. */ > +}; > + > #endif /* openflow/openflow-1.5.h */ > diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h > index 34708f3bd846..49b0f7a2cfa8 100644 > --- a/include/openvswitch/ofp-msgs.h > +++ b/include/openvswitch/ofp-msgs.h > @@ -287,6 +287,8 @@ enum ofpraw { > OFPRAW_OFPST10_FLOW_REQUEST, > /* OFPST 1.1+ (1): struct ofp11_flow_stats_request, uint8_t[8][]. */ > OFPRAW_OFPST11_FLOW_REQUEST, > + /* OFPST 1.5+ (17): struct ofp15_oxs_flow_stats_request, uint8_t[8][]. */ > + OFPRAW_OFPST15_OXS_FLOW_REQUEST, > /* NXST 1.0 (0): struct nx_flow_stats_request, uint8_t[8][]. */ > OFPRAW_NXST_FLOW_REQUEST, > > @@ -296,6 +298,8 @@ enum ofpraw { > OFPRAW_OFPST11_FLOW_REPLY, > /* OFPST 1.3+ (1): uint8_t[]. */ > OFPRAW_OFPST13_FLOW_REPLY, > + /* OFPST 1.5+ (17): uint8_t[]. */ > + OFPRAW_OFPST15_OXS_FLOW_REPLY, > /* NXST 1.0 (0): uint8_t[]. */ > OFPRAW_NXST_FLOW_REPLY, > > @@ -628,10 +632,12 @@ enum ofptype { > OFPTYPE_FLOW_STATS_REQUEST, /* OFPRAW_OFPST10_FLOW_REQUEST. > * OFPRAW_OFPST11_FLOW_REQUEST. > * OFPRAW_NXST_FLOW_REQUEST. */ > + OFPTYPE_OXS_FLOW_STATS_REQUEST, /* OFPRAW_OFPST15_OXS_FLOW_REQUEST. */ > OFPTYPE_FLOW_STATS_REPLY, /* OFPRAW_OFPST10_FLOW_REPLY. > * OFPRAW_OFPST11_FLOW_REPLY. > * OFPRAW_OFPST13_FLOW_REPLY. > * OFPRAW_NXST_FLOW_REPLY. */ > + OFPTYPE_OXS_FLOW_STATS_REPLY, /* OFPRAW_OFPST15_OXS_FLOW_REPLY. */ > OFPTYPE_AGGREGATE_STATS_REQUEST, /* OFPRAW_OFPST10_AGGREGATE_REQUEST. > * OFPRAW_OFPST11_AGGREGATE_REQUEST. > * OFPRAW_NXST_AGGREGATE_REQUEST. */ > diff --git a/lib/automake.mk b/lib/automake.mk > index faace7993e79..06ba35a43d08 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -197,6 +197,8 @@ lib_libopenvswitch_la_SOURCES = \ > lib/ovsdb-parser.h \ > lib/ovsdb-types.c \ > lib/ovsdb-types.h \ > + lib/ox-stat.c \ > + lib/ox-stat.h \ Please line this up with the previous lines. > lib/packets.c \ > lib/packets.h \ > lib/pcap-file.c \ > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c > index c8cac5b4765c..5bfab5f8ece8 100644 > --- a/lib/ofp-parse.c > +++ b/lib/ofp-parse.c > @@ -41,6 +41,8 @@ > #include "socket-util.h" > #include "util.h" > > +extern uint8_t oxs_field_set; > + > /* Parses 'str' as an 8-bit unsigned integer into '*valuep'. > * > * 'name' describes the value parsed in an error message, if any. > @@ -188,6 +190,34 @@ str_to_connhelper(const char *str, uint16_t *alg) > return xasprintf("invalid conntrack helper \"%s\"", str); > } > > +struct ox_fields { > + const char *name; > + uint16_t fl_type; > +}; > + > +static bool > +parse_oxs_field(const char *name, const struct ox_fields **f_out) > +{ > + static const struct ox_fields fields[] = { > + { "oxs-duration", OFPXST_OFB_DURATION }, > + { "oxs-idle_time", OFPXST_OFB_IDLE_TIME }, > + { "oxs-flow_count", OFPXST_OFB_FLOW_COUNT }, > + { "oxs-packet_count", OFPXST_OFB_PACKET_COUNT }, > + { "oxs-byte_count", OFPXST_OFB_BYTE_COUNT }, > + }; > + > + const struct ox_fields *f; > + > + for (f = fields; f < &fields[ARRAY_SIZE(fields)]; f++) { > + if (!strcmp(f->name, name)) { > + *f_out = f; > + return true; > + } > + } > + *f_out = NULL; > + return false; > +} These indent levels should be 4-spaces, per the coding standard. > + > struct protocol { > const char *name; > uint16_t dl_type; > @@ -406,10 +436,23 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int > command, char *string, > > while (ofputil_parse_key_value(&string, &name, &value)) { > const struct protocol *p; > + const struct ox_fields *f; > const struct mf_field *mf; > char *error = NULL; > > - if (parse_protocol(name, &p)) { > + if (parse_oxs_field(name, &f)) { > + if(f->fl_type == OFPXST_OFB_DURATION) { > + oxs_field_set |= 1<<0; > + } else if(f->fl_type == OFPXST_OFB_IDLE_TIME) { > + oxs_field_set |= 1<<1; > + } else if(f->fl_type == OFPXST_OFB_FLOW_COUNT) { > + oxs_field_set |= 1<<2; > + } else if(f->fl_type == OFPXST_OFB_PACKET_COUNT) { > + oxs_field_set |= 1<<3; > + } else if(f->fl_type == OFPXST_OFB_BYTE_COUNT) { > + oxs_field_set |= 1<<4; > + } Would this block be better written as: if (parse_oxs_field(name, &f)) { oxs_field_set |= (1 << f->fl_type); } else if (parse_protocol(.... I don't know if the explicitness really is needed. Even if not, please put a space around the operators (you have 1<<X, make it 1 << X). > + } else if (parse_protocol(name, &p)) { > match_set_dl_type(&fm->match, htons(p->dl_type)); > if (p->nw_proto) { > match_set_nw_proto(&fm->match, p->nw_proto); > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index 7ca953100539..0f855fab5481 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -3563,6 +3563,11 @@ ofp_to_string__(const struct ofp_header *oh, enum > ofpraw raw, > ofp_print_flow_stats_request(string, oh); > break; > > + case OFPTYPE_OXS_FLOW_STATS_REQUEST: > + ofp_print_stats(string, oh); > + ofp_print_flow_stats_request(string, oh); > + break; > + > case OFPTYPE_TABLE_STATS_REQUEST: > ofp_print_stats(string, oh); > break; > @@ -3587,6 +3592,11 @@ ofp_to_string__(const struct ofp_header *oh, enum > ofpraw raw, > ofp_print_flow_stats_reply(string, oh); > break; > > + case OFPTYPE_OXS_FLOW_STATS_REPLY: > + ofp_print_stats(string, oh); > + ofp_print_flow_stats_reply(string, oh); > + break; > + > case OFPTYPE_QUEUE_STATS_REPLY: > ofp_print_stats(string, oh); > ofp_print_ofpst_queue_reply(string, oh, verbosity); > diff --git a/lib/rconn.c b/lib/rconn.c > index 8a2986403efd..13890e743bc2 100644 > --- a/lib/rconn.c > +++ b/lib/rconn.c > @@ -1392,6 +1392,8 @@ is_admitted_msg(const struct ofpbuf *b) > case OFPTYPE_DESC_STATS_REPLY: > case OFPTYPE_FLOW_STATS_REQUEST: > case OFPTYPE_FLOW_STATS_REPLY: > + case OFPTYPE_OXS_FLOW_STATS_REQUEST: > + case OFPTYPE_OXS_FLOW_STATS_REPLY: > case OFPTYPE_AGGREGATE_STATS_REQUEST: > case OFPTYPE_AGGREGATE_STATS_REPLY: > case OFPTYPE_TABLE_STATS_REQUEST: > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index d5410fd1b20f..4b95c9d2f8f7 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -8092,6 +8092,9 @@ handle_openflow__(struct ofconn *ofconn, const struct > ofpbuf *msg) > case OFPTYPE_FLOW_STATS_REQUEST: > return handle_flow_stats_request(ofconn, oh); > > + case OFPTYPE_OXS_FLOW_STATS_REQUEST: > + return handle_flow_stats_request(ofconn, oh); > + > case OFPTYPE_AGGREGATE_STATS_REQUEST: > return handle_aggregate_stats_request(ofconn, oh); > > @@ -8167,6 +8170,7 @@ handle_openflow__(struct ofconn *ofconn, const struct > ofpbuf *msg) > case OFPTYPE_QUEUE_GET_CONFIG_REPLY: > case OFPTYPE_DESC_STATS_REPLY: > case OFPTYPE_FLOW_STATS_REPLY: > + case OFPTYPE_OXS_FLOW_STATS_REPLY: > case OFPTYPE_QUEUE_STATS_REPLY: > case OFPTYPE_PORT_STATS_REPLY: > case OFPTYPE_TABLE_STATS_REPLY: =====-----=====-----===== 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev