[ovs-dev] [PATCH 5/6] Implement OpenFlow 1.4+ OFPTC_EVICTION.

Ben Pfaff blp at nicira.com
Fri Jul 3 03:31:44 UTC 2015


On Thu, Jul 02, 2015 at 12:01:32PM -0700, Jarno Rajahalme wrote:
> 
> With some questions for clarification:
> 
> Acked-by: Jarno Rajahalme <jrajahalme at 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 <blp at 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.



More information about the dev mailing list