[ovs-dev] [cleanups 05/13] ofproto: Break subrules out of "struct rule" into new "struct facet".

Ben Pfaff blp at nicira.com
Fri Nov 12 22:50:37 UTC 2010


On Fri, Nov 12, 2010 at 11:28:01AM -0800, Ethan Jackson wrote:
> This is a major improvement in the openflow code.  It's definitely
> going to be easier to reason about in the future.

Thanks.

> > +    /* Figure out what we need to revalidate now, if anything. */
> > +    struct tag_set revalidate_set = p->revalidate_set;
> > +    if (p->need_revalidate) {
> > +        revalidate_all = true;
> > +    }
> > +
> > +    /* Clear the revalidation flags. */
> > +    tag_set_init(&p->revalidate_set);
> > +    p->need_revalidate = false;
> > +
> > +    /* Now revalidate if there's anything to do. */
> > +    if (revalidate_all || !tag_set_is_empty(&p->revalidate_set)) {
> > +        struct facet *facet, *next;
> > +
> > +        HMAP_FOR_EACH_SAFE (facet, next, hmap_node, &p->facets) {
> > +            if (revalidate_all
> > +                || tag_set_intersects(&revalidate_set, facet->tags)) {
> > +                facet_revalidate(p, facet);
> > +            }
> > +        }
> >     }
> >
> >     return 0;
> Don't we want to tag_set_init(&p->revalidate_set) after the if
> statement?  tag_set_is_empty is always going to be true in this case.

Good spotting.

The 'if' statement was actually supposed to check the local copy of the
revalidate_set, not the one in 'p'.

> > @@ -2024,270 +2079,332 @@ rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet,
> >  {
> >     struct rule *displaced_rule;
> >
> > -    /* Insert the rule in the classifier. */
> >     displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
> > -    if (!rule->cr.wc.wildcards) {
> > -        rule_make_actions(p, rule, packet);
> > +    if (displaced_rule) {
> > +        rule_destroy(p, displaced_rule);
> >     }
> > +    p->need_revalidate = true;
> >
> > -    /* Send the packet and credit it to the rule. */
> >     if (packet) {
> > -        struct flow flow;
> > -        flow_extract(packet, 0, in_port, &flow);
> > -        rule_execute(p, rule, packet, &flow);
> > -    }
> > -
> > -    /* Install the rule in the datapath only after sending the packet, to
> > -     * avoid packet reordering.  */
> > -    if (rule->cr.wc.wildcards) {
> > -        COVERAGE_INC(ofproto_add_wc_flow);
> > -        p->need_revalidate = true;
> > -    } else {
> > -        rule_install(p, rule, displaced_rule);
> > -    }
> > -
> > -    /* Free the rule that was displaced, if any. */
> > -    if (displaced_rule) {
> > -        rule_destroy(p, displaced_rule);
> > +        rule_execute(p, rule, in_port, packet);
> >     }
> >  }
> 
> Why does rule_insert optionally send the packet?  [...]

There is no good reason any longer.  At one point the code was factored
differently so that there was a reason, but I can't remember the details
anymore.

I've added a commit that cleans this up, here it is:

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Fri, 12 Nov 2010 14:49:46 -0800
Subject: [PATCH] ofproto: Simplify rule_insert().

There's no reason for the 'packet' and 'in_port' arguments; the caller can
call rule_execute() just as easily as rule_insert() can.

Suggested-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 14570be..61ef581 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -124,8 +124,7 @@ static void rule_destroy(struct ofproto *, struct rule *);
 static void rule_free(struct rule *);
 
 static struct rule *rule_lookup(struct ofproto *, const struct flow *);
-static void rule_insert(struct ofproto *, struct rule *,
-                        struct ofpbuf *packet, uint16_t in_port);
+static void rule_insert(struct ofproto *, struct rule *);
 static void rule_remove(struct ofproto *, struct rule *);
 
 static void rule_send_removed(struct ofproto *, struct rule *, uint8_t reason);
@@ -1338,7 +1337,7 @@ ofproto_add_flow(struct ofproto *p, const struct cls_rule *cls_rule,
 {
     struct rule *rule;
     rule = rule_create(cls_rule, actions, n_actions, 0, 0, 0, false);
-    rule_insert(p, rule, NULL, 0);
+    rule_insert(p, rule);
 }
 
 void
@@ -2067,15 +2066,9 @@ rule_execute(struct ofproto *ofproto, struct rule *rule, uint16_t in_port,
     }
 }
 
-/* Inserts 'rule' into 'p''s flow table.
- *
- * If 'packet' is nonnull, takes ownership of 'packet', executes 'rule''s
- * actions on it and credits the statistics for sending the packet to 'rule'.
- * 'packet' must have at least sizeof(struct ofp_packet_in) bytes of
- * headroom. */
+/* Inserts 'rule' into 'p''s flow table. */
 static void
-rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet,
-            uint16_t in_port)
+rule_insert(struct ofproto *p, struct rule *rule)
 {
     struct rule *displaced_rule;
 
@@ -2084,10 +2077,6 @@ rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet,
         rule_destroy(p, displaced_rule);
     }
     p->need_revalidate = true;
-
-    if (packet) {
-        rule_execute(p, rule, in_port, packet);
-    }
 }
 
 /* Creates and returns a new facet within 'ofproto' owned by 'rule', given a
@@ -3939,7 +3928,10 @@ add_flow(struct ofconn *ofconn, struct flow_mod *fm)
     rule = rule_create(&fm->cr, fm->actions, fm->n_actions,
                        fm->idle_timeout, fm->hard_timeout, fm->cookie,
                        fm->flags & OFPFF_SEND_FLOW_REM);
-    rule_insert(p, rule, packet, in_port);
+    rule_insert(p, rule);
+    if (packet) {
+        rule_execute(p, rule, in_port, packet);
+    }
     return error;
 }
 
-- 
1.7.1





More information about the dev mailing list