[ovs-dev] [PATCH 1/2] OF1.5/EXT-334 OXS/Individal Flow Statistics

Aaron Conole aconole at redhat.com
Thu May 11 17:53:19 UTC 2017


Hi Satya,

I haven't checked the OF1.5 spec with this, yet.  Just saw a few things.

Thanks for the contribution!

Ben Pfaff <blp at ovn.org> writes:

> From: Satya Valli <satyavalli.rama at 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:


More information about the dev mailing list