On Thu, Jul 02, 2015 at 12:01:32PM -0700, Jarno Rajahalme wrote:
>
> With some questions for clarification:
>
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
Thanks for the review! Responses below. I'll just snip the ones the
obvious fixes that I applied.
> > On Jun 24, 2015, at 10:57 AM, Ben Pfaff <b...@nicira.com> wrote:
> > +static void
> > +table_mod__(struct oftable *oftable,
> > + enum ofputil_table_miss miss, enum ofputil_table_eviction
> > eviction)
> > +{
> > + if (miss != OFPUTIL_TABLE_MISS_DEFAULT) {
> > + atomic_store_relaxed(&oftable->miss_config, miss);
> > + }
> > +
>
> Would this not allow miss_config to be changed back to MISS_DEFAULT?
> If this is intentional, could you add a comment?
Like this?
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 5a682bf..3615dc4 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6568,7 +6568,11 @@ static void
table_mod__(struct oftable *oftable,
enum ofputil_table_miss miss, enum ofputil_table_eviction eviction)
{
- if (miss != OFPUTIL_TABLE_MISS_DEFAULT) {
+ if (miss == OFPUTIL_TABLE_MISS_DEFAULT) {
+ /* This is how an OFPT_TABLE_MOD decodes if it doesn't specify any
+ * table-miss configuration (because the protocol used doesn't have
+ * such a concept), so there's nothing to do. */
+ } else {
atomic_store_relaxed(&oftable->miss_config, miss);
}
> > static enum ofperr
> > @@ -6561,18 +6582,33 @@ table_mod(struct ofproto *ofproto, const struct
> > ofputil_table_mod *tm)
> > {
> > if (!check_table_id(ofproto, tm->table_id)) {
> > return OFPERR_OFPTMFC_BAD_TABLE;
> > - } else if (tm->miss_config != OFPUTIL_TABLE_MISS_DEFAULT) {
> > - if (tm->table_id == OFPTT_ALL) {
> > - int i;
> > - for (i = 0; i < ofproto->n_tables; i++) {
> > - atomic_store_relaxed(&ofproto->tables[i].miss_config,
> > - tm->miss_config);
> > + }
> > +
> > + /* Don't allow the eviction flags to be changed. OF1.4 says this is
> > + * normal: "The OFPTMPT_EVICTION property usually cannot be modified
> > using
> > + * a OFP_TABLE_MOD request, because the eviction mechanism is switch
> > + * defined". */
> > + if (tm->eviction_flags != UINT32_MAX
> > + && tm->eviction_flags != (OFPTMPEF14_OTHER | OFPTMPEF14_IMPORTANCE
> > + | OFPTMPEF14_LIFETIME)) {
>
> Why is this one specific combination of three flags allowed?
That's the only kind of eviction that the OVS code implements.
I should break this out as a #define, since in the next commit we'll
need the same thing in another place. OK, like this then:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 5a682bf..970aca3 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -90,6 +90,12 @@ static void oftable_configure_eviction(struct oftable *,
size_t n_fields)
OVS_REQUIRES(ofproto_mutex);
+/* This is the only combination of OpenFlow eviction flags that OVS supports: a
+ * combination of OF1.4+ importance, the remaining lifetime of the flow, and
+ * fairness based on user-specified fields. */
+#define OFPROTO_EVICTION_FLAGS \
+ (OFPTMPEF14_OTHER | OFPTMPEF14_IMPORTANCE | OFPTMPEF14_LIFETIME)
+
/* A set of rules within a single OpenFlow table (oftable) that have the same
* values for the oftable's eviction_fields. A rule to be evicted, when one is
* needed, is taken from the eviction group that contains the greatest number
@@ -6595,13 +6605,13 @@ table_mod(struct ofproto *ofproto, const struct
ofputil_table_mod *tm)
return OFPERR_OFPTMFC_BAD_TABLE;
}
- /* Don't allow the eviction flags to be changed. OF1.4 says this is
- * normal: "The OFPTMPT_EVICTION property usually cannot be modified using
- * a OFP_TABLE_MOD request, because the eviction mechanism is switch
+ /* Don't allow the eviction flags to be changed (except to the only fixed
+ * value that OVS supports). OF1.4 says this is normal: "The
+ * OFPTMPT_EVICTION property usually cannot be modified using a
+ * OFP_TABLE_MOD request, because the eviction mechanism is switch
* defined". */
if (tm->eviction_flags != UINT32_MAX
- && tm->eviction_flags != (OFPTMPEF14_OTHER | OFPTMPEF14_IMPORTANCE
- | OFPTMPEF14_LIFETIME)) {
+ && tm->eviction_flags != OFPROTO_EVICTION_FLAGS) {
return OFPERR_OFPTMFC_BAD_CONFIG;
}
I'll use OFPROTO_EVICTION_FLAGS in the next commit too.
> > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> > index 63c2ecc..92edd91 100644
> > --- a/utilities/ovs-ofctl.8.in
> > +++ b/utilities/ovs-ofctl.8.in
> > @@ -62,20 +62,6 @@ Prints to the console statistics for each of the flow
> > tables used by
> > \fBdump\-table\-features \fIswitch\fR
> > Prints to the console features for each of the flow tables used by
> > \fIswitch\fR.
> > -.
> > -.IP "\fBmod\-table \fIswitch\fR \fItable_id\fR \fIflow_miss_handling\fR"
> > -An OpenFlow 1.0 switch looks up each packet that arrives at the switch
> > -in table 0, then in table 1 if there is no match in table 0, then in
> > -table 2, and so on until the packet finds a match in some table.
> > -Finally, if no match was found, the switch sends the packet to the
> > -controller
> > -.IP
> > -OpenFlow 1.1 and later offer more flexibility. This command
> > -configures the flow table miss handling configuration for table
> > -\fItable_id\fR in \fIswitch\fR. \fItable_id\fR may be an OpenFlow
> > -table number between 0 and 254, inclusive, or the keyword \fBALL\fR to
> > -modify all tables. \fIflow_miss_handling\fR may be any one of the
> > -following:
>
> Why was this documentation removed?
I incorrectly broke up the changes to ovs-ofctl(8) between this and the
following commit. I'll fix that, thanks for pointing it it out.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
