On Fri, Aug 31, 2012 at 05:42:48PM +0900, Simon Horman wrote: > On Thu, Aug 30, 2012 at 10:02:51AM +0900, Simon Horman wrote: > > On Wed, Aug 29, 2012 at 10:07:55AM -0700, Ben Pfaff wrote: > > > On Tue, Aug 21, 2012 at 01:55:36PM +0900, Simon Horman wrote: > > > > Signed-off-by: Simon Horman <[email protected]> > > > > > > I'm not really happy with this approach. It seems fairly awkward. I > > > would much rather define some "struct ofputil_*" that has a superset of > > > the 1.[012] table stats and define converter functions. > > > > Sure, I'll see what I can do. > > I do agree that the approach I have taken is somewhat awkward. > > Hi Ben, > > is this closer to what you would like?
Hi Ben, could you take a moment to look over this? I'd like to know if this approach is appropriate. > From: Simon Horman <[email protected]> > > [PATCH] ofp-util: Allow decoding of Open Flow 1.1 & 1.2 Table Statistics > Request Messages > > Signed-off-by: Simon Horman <[email protected]> > > --- > > v13 (draft) > * Don't make use of a union of different ofpX_table_stats structures. > Rather, use ofp12_table_stats and convert as necessary. > > v12 > * No change > > v11 > * No change > > v10 > * No change > > v9 > * Set wildcards, match, write_setfields and apply_setfields based > on a bitmap of (1 << OFPXMT_*) > > v8 > * Manual rebase > * Make use of enum ofp_version > * Add ofp-tests > > v7 > * Omitted > > v6 > * No change > > v5 > * Manual rebase > * Add OFPST_TABLE entry for Open Flow 1.1 and 1.2 to ofputil_msg_types, > this wires-up decoding of table statistics messages. > > v4 > * Initial post > > table test > --- > include/openflow/openflow-1.1.h | 4 ++ > include/openflow/openflow-1.2.h | 5 +++ > ofproto/ofproto-dpif.c | 8 ++-- > ofproto/ofproto-provider.h | 40 ++++++++++++++++-- > ofproto/ofproto.c | 94 > +++++++++++++++++++++++++++++++++++++++-- > tests/ofp-print.at | 16 ++++++- > 6 files changed, 156 insertions(+), 11 deletions(-) > > diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h > index 696c3ec..5592520 100644 > --- a/include/openflow/openflow-1.1.h > +++ b/include/openflow/openflow-1.1.h > @@ -281,6 +281,10 @@ enum ofp11_instruction_type { > OFPIT11_EXPERIMENTER = 0xFFFF /* Experimenter instruction */ > }; > > +#define OFPIT11_ALL OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \ > + OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS | \ > + OFPIT11_CLEAR_ACTIONS > + > #define OFP11_INSTRUCTION_ALIGN 8 > > /* Generic ofp_instruction structure. */ > diff --git a/include/openflow/openflow-1.2.h b/include/openflow/openflow-1.2.h > index 0a73ed1..64bc993 100644 > --- a/include/openflow/openflow-1.2.h > +++ b/include/openflow/openflow-1.2.h > @@ -106,8 +106,13 @@ enum oxm12_ofb_match_fields { > OFPXMT12_OFB_IPV6_ND_TLL, /* Target link-layer for ND. */ > OFPXMT12_OFB_MPLS_LABEL, /* MPLS label. */ > OFPXMT12_OFB_MPLS_TC, /* MPLS TC. */ > + > + /* End Marker */ > + OFPXMT12_OFB_MAX, > }; > > +#define OFPXMT12_MASK ((1ULL << OFPXMT12_OFB_MAX) - 1) > + > /* OXM implementation makes use of NXM as they are the same format > * with different field definitions > */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 78cb186..2dd7eee 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1156,7 +1156,8 @@ get_features(struct ofproto *ofproto_ OVS_UNUSED, > } > > static void > -get_tables(struct ofproto *ofproto_, struct ofp10_table_stats *ots) > +get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots, > + enum ofp_version ofp_version OVS_UNUSED) > { > struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); > struct dpif_dp_stats s; > @@ -1164,9 +1165,8 @@ get_tables(struct ofproto *ofproto_, struct > ofp10_table_stats *ots) > strcpy(ots->name, "classifier"); > > dpif_get_dp_stats(ofproto->dpif, &s); > - put_32aligned_be64(&ots->lookup_count, htonll(s.n_hit + s.n_missed)); > - put_32aligned_be64(&ots->matched_count, > - htonll(s.n_hit + ofproto->n_matches)); > + ots->lookup_count = htonll(s.n_hit + s.n_missed); > + ots->matched_count = htonll(s.n_hit + ofproto->n_matches); > } > > static struct ofport * > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 15dc347..690a933 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -456,7 +456,25 @@ struct ofproto_class { > * > * - 'name' to "table#" where # is the table ID. > * > - * - 'wildcards' to OFPFW10_ALL. > + * - 'wildcards' to OFPFW10_ALL (OpenFlow 1.0) or > + * OFPFW11_ALL (OpenFlow 1.1 and 1.2). > + * > + * - 'instructions' to OFPIT11_ALL (OpenFlow 1.1 and 1.2). > + * Not used in OpenFlow 1.0. > + * > + * - 'write_actions' to OFPAT11_OUTPUT (OpenFlow 1.1) or > + * OFPAT12_OUTPUT (OpenFlow 1.2). > + * Not present in OpenFLow 1.0. > + * > + * - 'apply_actions' to OFPAT11_OUTPUT (OpenFlow 1.1) or > + * OFPAT12_OUTPUT (OpenFlow 1.2). > + * Not used in OpenFLow 1.0. > + * > + * - 'write_setfields' to OFPXMT12_SUPPORTED (OpenFlow 1.2). > + * Not used in OpenFLow 1.0 or 1.1. > + * > + * - 'apply_setfields' to OFPXMT12_SUPPORTED (OpenFlow 1.2). > + * Not used in OpenFlow 1.0 or 1.1. > * > * - 'max_entries' to 1,000,000. > * > @@ -472,6 +490,21 @@ struct ofproto_class { > * - 'wildcards' to the set of wildcards actually supported by the > table > * (if it doesn't support all OpenFlow wildcards). > * > + * - 'instructions' to set the instructions actually supported by > + * the table. > + * > + * - 'write_actions' to set the write actions actually supported by > + * the table (if it doesn't support all OpenFlow actions). > + * > + * - 'apply_actions' to set the apply actions actually supported by > + * the table (if it doesn't support all OpenFlow actions). > + * > + * - 'write_setfields' to set the write setfields actually supported by > + * the table. > + * > + * - 'apply_setfields' to set the apply setfields actually supported by > + * the table. > + * > * - 'max_entries' to the maximum number of flows actually supported by > * the hardware. > * > @@ -481,10 +514,11 @@ struct ofproto_class { > * - 'matched_count' to the number of packets looked up in this flow > * table so far that matched one of the flow entries. > * > - * Keep in mind that all of the members of struct ofp10_table_stats are > in > + * Keep in mind that all of the members of struct ofp12_table_stats are > in > * network byte order. > */ > - void (*get_tables)(struct ofproto *ofproto, struct ofp10_table_stats > *ots); > + void (*get_tables)(struct ofproto *ofproto, struct ofp12_table_stats > *ots, > + enum ofp_version ofp_version); > > /* ## ---------------- ## */ > /* ## ofport Functions ## */ > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 5c9ab9d..4fe0ca1 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -2208,21 +2208,49 @@ handle_table_stats_request(struct ofconn *ofconn, > const struct ofp_header *request) > { > struct ofproto *p = ofconn_get_ofproto(ofconn); > - struct ofp10_table_stats *ots; > + struct ofp12_table_stats *ots; > struct ofpbuf *msg; > size_t i; > > + /* Set up default values. > + * > + * ofp12_table_stats is used as a generic structure as > + * it is able to hold all the fields for ofp10_table_stats > + * and ofp11_table_stats (and of course itself). > + */ > msg = ofpraw_alloc_stats_reply(request, sizeof *ots * p->n_tables); > ots = ofpbuf_put_zeros(msg, sizeof *ots * p->n_tables); > for (i = 0; i < p->n_tables; i++) { > ots[i].table_id = i; > sprintf(ots[i].name, "table%zu", i); > - ots[i].wildcards = htonl(OFPFW10_ALL); > ots[i].max_entries = htonl(1000000); /* An arbitrary big number. */ > ots[i].active_count = htonl(classifier_count(&p->tables[i].cls)); > + ots[i].config = ots[i].apply_actions = htonl(0); > + > + switch (request->version) { > + case OFP12_VERSION: > + ots[i].wildcards = ots[i].match = ots[i].write_setfields = > + ots[i].apply_setfields = htonll(OFPXMT12_MASK); > + ots[i].write_actions = ots[i].apply_actions = > htonl(OFPAT12_OUTPUT); > + ots[i].instructions = htonl(OFPIT11_ALL); > + break; > + > + case OFP11_VERSION: > + ots[i].wildcards = ots[i].match = htonll(OFPFW11_ALL); > + ots[i].instructions = htonl(OFPIT11_ALL); > + ots[i].write_actions = ots[i].apply_actions = > htonl(OFPAT11_OUTPUT); > + break; > + > + case OFP10_VERSION: > + ots[i].wildcards = htonll(OFPFW10_ALL); > + break; > + > + default: > + NOT_REACHED(); > + } > } > > - p->ofproto_class->get_tables(p, ots); > + p->ofproto_class->get_tables(p, ots, request->version); > > for (i = 0; i < p->n_tables; i++) { > const struct oftable *table = &p->tables[i]; > @@ -2236,6 +2264,66 @@ handle_table_stats_request(struct ofconn *ofconn, > } > } > > + /* Convert from ofp12_table_stats as necessary */ > + switch (request->version) { > + case OFP12_VERSION: > + break; > + > + case OFP11_VERSION: { > + struct ofp11_table_stats *ots11; > + struct ofpbuf *new_msg; > + > + new_msg = ofpraw_alloc_stats_reply(request, > + sizeof *ots11 * p->n_tables); > + ots11 = ofpbuf_put_zeros(new_msg, sizeof *ots11 * p->n_tables); > + > + for (i = 0; i < p->n_tables; i++) { > + ots11[i].table_id = ots[i].table_id; > + strcpy(ots11[i].name, ots[i].name); > + ots11[i].wildcards = htonl(ntohll(ots[i].wildcards)); > + ots11[i].match = htonl(ntohll(ots[i].match)); > + ots11[i].instructions = ots[i].instructions; > + ots11[i].write_actions = ots[i].write_actions; > + ots11[i].apply_actions = ots[i].apply_actions; > + ots11[i].config = ots[i].config; > + ots11[i].max_entries = ots[i].max_entries; > + ots11[i].active_count = ots[i].active_count; > + ots11[i].lookup_count = ots[i].lookup_count; > + ots11[i].matched_count = ots[i].matched_count; > + } > + > + ofpbuf_delete(msg); > + msg = new_msg; > + break; > + } > + > + case OFP10_VERSION: { > + struct ofp10_table_stats *ots10; > + struct ofpbuf *new_msg; > + > + new_msg = ofpraw_alloc_stats_reply(request, > + sizeof *ots10 * p->n_tables); > + ots10 = ofpbuf_put_zeros(new_msg, sizeof *ots10 * p->n_tables); > + > + for (i = 0; i < p->n_tables; i++) { > + ots10[i].table_id = ots[i].table_id; > + strcpy(ots10[i].name, ots[i].name); > + ots10[i].wildcards = htonl(ntohll(ots[i].wildcards)); > + ots10[i].max_entries = ots[i].max_entries; > + ots10[i].active_count = ots[i].active_count; > + put_32aligned_be64(&ots10[i].lookup_count, ots[i].lookup_count); > + put_32aligned_be64(&ots10[i].matched_count, > ots[i].matched_count); > + } > + > + ofpbuf_delete(msg); > + msg = new_msg; > + break; > + } > + > + default: > + NOT_REACHED(); > + } > + > ofconn_send_reply(ofconn, msg); > return 0; > } > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > index 3c55d91..ca7cf6b 100644 > --- a/tests/ofp-print.at > +++ b/tests/ofp-print.at > @@ -794,13 +794,27 @@ OFPST_AGGREGATE reply (OF1.2) (xid=0x2): > packet_count=121 byte_count=19279 flow_ > ]) > AT_CLEANUP > > -AT_SETUP([OFPST_TABLE request]) > +AT_SETUP([OFPST_TABLE request - OF1.0]) > AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST]) > AT_CHECK([ovs-ofctl ofp-print "0110000c0000000100030000"], [0], [dnl > OFPST_TABLE request (xid=0x1): > ]) > AT_CLEANUP > > +AT_SETUP([OFPST_TABLE request - OF1.1]) > +AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST]) > +AT_CHECK([ovs-ofctl ofp-print "02120010000000020003000000000000"], [0], [dnl > +OFPST_TABLE request (OF1.1) (xid=0x2): > +]) > +AT_CLEANUP > + > +AT_SETUP([OFPST_TABLE request - OF1.2]) > +AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST]) > +AT_CHECK([ovs-ofctl ofp-print "03120010000000020003000000000000"], [0], [dnl > +OFPST_TABLE request (OF1.2) (xid=0x2): > +]) > +AT_CLEANUP > + > AT_SETUP([OFPST_TABLE reply - OF1.0]) > AT_KEYWORDS([ofp-print OFPT_STATS_REPLY]) > AT_CHECK([ovs-ofctl ofp-print "\ > -- > 1.7.10.2.484.gcd07cc5 > > > > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
