[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