[ovs-dev] [PATCH 2/2] ofproto-dpif: Store relevant fields for wildcarding in facet.

Justin Pettit jpettit at nicira.com
Tue Jun 11 19:01:24 UTC 2013


On Jun 11, 2013, at 9:41 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Tue, Jun 11, 2013 at 01:20:32AM -0700, Justin Pettit wrote:
>> Dynamically determines the flow fields that were relevant in
>> processing flows based on the OpenFlow flow table and switch
>> configuration.  The immediate use for this functionality is to
>> cache action translations for similar flows in facets.  This yields
>> a roughly 80% improvement in flow set up rates for a complicated
>> flow table.
>> 
>> More importantly, these wildcards will be used to determine what to
>> wildcard for the forthcoming kernel wildcard (megaflow) patches
>> that will allow wildcarding in the kernel, which will provide
>> significant flow set up improvements.
>> 
>> The approach to tracking fields and caching action translations in
>> facets was based on an impressive prototype by Ethan Jackson.
>> 
>> Co-authored-by: Ethan Jackson <ethan at nicira.com>
>> Signed-off-by: Ethan Jackson <ethan at nicira.com>
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> 
> struct facet needs a comment update.

Oh, good point.

> In handle_flow_miss_without_facet(), the declaration of 'op' can be
> moved inward.

Done.

> Is struct facet's new 'match' member good for anything?  The only use I
> see, in facet_create(), could be replaced by a local variable in that
> function.

Fixed.

> facet_lookup_valid()'s comment needs an update.

Done.

> The first loop in facet_revalidate() looks wrong to me.  I don't think
> that that the subfacets would usually have flows that are bitwise
> exactly equal (with memcmp()) to the facet's flow when wildcards are
> involved:
> 
>    /* Check that child subfacets still correspond to this facet.  Tunnel
>     * configuration changes could cause a subfacet's OpenFlow in_port to
>     * change. */
>    LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
>        struct ofproto_dpif *recv_ofproto;
>        struct flow recv_flow;
>        int error;
> 
>        error = ofproto_receive(ofproto->backer, NULL, subfacet->key,
>                                subfacet->key_len, &recv_flow, NULL,
>                                &recv_ofproto, NULL, NULL);
>        if (error
>            || recv_ofproto != ofproto
>            || memcmp(&recv_flow, &facet->flow, sizeof recv_flow)) {
>            facet_remove(facet);
>            return false;
>        }
>    }

I replaced the check with a facet_find() and compare the result with the current facet.

An incremental follows.

--Justin


-=-=-=-=-=-=-=-=-=-=-=-

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7442642..8df2679 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -435,20 +435,26 @@ static int subfacet_install(struct subfacet *,
                             struct dpif_flow_stats *);
 static void subfacet_uninstall(struct subfacet *);
 
-/* An exact-match instantiation of an OpenFlow flow.
+/* A unique, non-overlapping instantiation of an OpenFlow flow.
  *
  * A facet associates a "struct flow", which represents the Open vSwitch
- * userspace idea of an exact-match flow, with one or more subfacets.  Each
- * subfacet tracks the datapath's idea of the exact-match flow equivalent to
- * the facet.  When the kernel module (or other dpif implementation) and Open
- * vSwitch userspace agree on the definition of a flow key, there is exactly
- * one subfacet per facet.  If the dpif implementation supports more-specific
- * flow matching than userspace, however, a facet can have more than one
- * subfacet, each of which corresponds to some distinction in flow that
- * userspace simply doesn't understand.
+ * userspace idea of an exact-match flow, with one or more subfacets.
+ * While the facet is created based on an exact-match flow, it is stored
+ * within the ofproto based on the wildcards that could be expressed
+ * based on the flow table and other configuration.  (See the 'wc'
+ * description in "struct xlate_out" for more details.)
  *
- * Flow expiration works in terms of subfacets, so a facet must have at least
- * one subfacet or it will never expire, leaking memory. */
+ * Each subfacet tracks the datapath's idea of the flow equivalent to
+ * the facet.  When the kernel module (or other dpif implementation) and
+ * Open vSwitch userspace agree on the definition of a flow key, there
+ * is exactly one subfacet per facet.  If the dpif implementation
+ * supports more-specific flow matching than userspace, however, a facet
+ * can have more than one subfacet.  Examples include the dpif
+ * implementation not supporting the same wildcards as userspace or some
+ * distinction in flow that userspace simply doesn't understand.
+ *
+ * Flow expiration works in terms of subfacets, so a facet must have at
+ * least one subfacet or it will never expire, leaking memory. */
 struct facet {
     /* Owners. */
     struct hmap_node hmap_node;  /* In owning ofproto's 'facets' hmap. */
@@ -461,7 +467,6 @@ struct facet {
 
     /* Key. */
     struct flow flow;           /* Flow of the creating subfacet. */
-    struct match match;         /* Match containing a flow with wildcards. */
     struct cls_rule cr;         /* In 'ofproto_dpif's facets classifier. */
 
     /* These statistics:
@@ -3742,7 +3747,6 @@ handle_flow_miss_without_facet(struct rule_dpif *rule, str
     struct ofpbuf *packet;
 
     LIST_FOR_EACH (packet, list_node, &miss->packets) {
-        struct flow_miss_op *op = &ops[*n_ops];
 
         COVERAGE_INC(facet_suppress);
 
@@ -3757,6 +3761,7 @@ handle_flow_miss_without_facet(struct rule_dpif *rule, str
         }
 
         if (xout->odp_actions.size) {
+            struct flow_miss_op *op = &ops[*n_ops];
             struct dpif_execute *execute = &op->dpif_op.u.execute;
 
             init_flow_miss_execute_op(miss, packet, op);
@@ -4711,6 +4716,7 @@ facet_create(const struct flow_miss *miss, struct rule_dpi
 {
     struct ofproto_dpif *ofproto = miss->ofproto;
     struct facet *facet;
+    struct match match;
 
     facet = xzalloc(sizeof *facet);
     facet->packet_count = facet->prev_packet_count = stats->n_packets;
@@ -4729,8 +4735,8 @@ facet_create(const struct flow_miss *miss, struct rule_dpi
 
     xlate_out_copy(&facet->xout, xout);
 
-    match_init(&facet->match, &facet->flow, &facet->xout.wc);
-    cls_rule_init(&facet->cr, &facet->match, OFP_DEFAULT_PRIORITY);
+    match_init(&match, &facet->flow, &facet->xout.wc);
+    cls_rule_init(&facet->cr, &match, OFP_DEFAULT_PRIORITY);
     classifier_insert(&ofproto->facets, &facet->cr);
 
     facet->nf_flow.output_iface = facet->xout.nf_output_iface;
@@ -4947,8 +4953,8 @@ facet_find(struct ofproto_dpif *ofproto, const struct flow
     return cr ? CONTAINER_OF(cr, struct facet, cr) : NULL;
 }
 
-/* Searches 'ofproto''s table of facets for one exactly equal to 'flow'.
- * Returns it if found, otherwise a null pointer.
+/* Searches 'ofproto''s table of facets for one capable that covers
+ * 'flow'.  Returns it if found, otherwise a null pointer.
  *
  * The returned facet is guaranteed to be valid. */
 static struct facet *
@@ -5074,7 +5080,7 @@ facet_revalidate(struct facet *facet)
                                 &recv_ofproto, NULL, NULL);
         if (error
             || recv_ofproto != ofproto
-            || memcmp(&recv_flow, &facet->flow, sizeof recv_flow)) {
+            || facet != facet_find(ofproto, &recv_flow)) {
             facet_remove(facet);
             return false;
         }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 177742f..e8458d0 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2217,7 +2217,6 @@ AT_BANNER([ofproto-dpif -- megaflows])
 # that vary from one run to another (e.g., timing and bond actions).
 m4_define([STRIP_XOUT], [[sed '
     s/used:[0-9]*\.[0-9]*/used:0.0/
-    s/miss:[0-9]*/miss:0/
     s/Datapath actions:.*/Datapath actions: <del>/
 ' | sort]])
 





More information about the dev mailing list