[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