On Jun 11, 2013, at 9:41 AM, Ben Pfaff <b...@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 <et...@nicira.com>
>> Signed-off-by: Ethan Jackson <et...@nicira.com>
>> Signed-off-by: Justin Pettit <jpet...@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]])
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
