From 38cc8bbe1fbec2ececd2b559721dc628a1b7623d Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 29 Apr 2009 15:36:44 -0700 Subject: [PATCH] secchan: Eliminate UNKNOWN_SUPER. When a super-rule is destroyed, secchan must reassess each of its subrules. Each subrule might now have no super-rule (which we suspect is the common case) or it might have a new super-rule. Until now, secchan has "optimized" this reassessment by initially assigning each of the deleted super-rule's subrules a super-rule of UNKNOWN_SUPER, which is not a valid rule at all. It did this in the hope that the subrule would get deleted before we need to know what its super-rule is. However, this has repeatedly led to bugs, since it's not always obvious what code will need to find a rule's super-rule. This commit fixes the problem by removing the "optimization" (in quotes because there is no evidence that it was a useful optimization in practice). --- secchan/ofproto.c | 77 ++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/secchan/ofproto.c b/secchan/ofproto.c index 9191ab6e..50b126aa 100644 --- a/secchan/ofproto.c +++ b/secchan/ofproto.c @@ -97,7 +97,6 @@ static int xlate_actions(const union ofp_action *in, size_t n_in, struct odp_actions *out, tag_type *tags, bool *may_setup_flow); -#define UNKNOWN_SUPER ((struct rule *)-1) struct rule { struct cls_rule cr; @@ -111,17 +110,14 @@ struct rule { uint8_t ip_tos; /* Last-seen IP type-of-service. */ tag_type tags; /* Tags (set only by hooks). */ - /* A subrule with a known super-rule has a non-null 'super' other than - * UNKNOWN_SUPER, in which case 'list' is an element of the super-rule's - * list of subrules. + /* If 'super' is non-NULL, this rule is a subrule, that is, it is an + * exact-match rule (having cr.wc.wildcards of 0) generated from the + * wildcard rule 'super'. In this case, 'list' is an element of the + * super-rule's list. * - * A subrule with an unknown super-rule (which is an optimization that - * might well be better removed) has 'super' set to UNKNOWN_SUPER, in - * which case 'list' is unused. - * - * A super-rule has 'super' of NULL, in which case 'list' is the head of a - * list of sub-rules. (A super-rule with no wildcards will never have any - * sub-rules.) */ + * If 'super' is NULL, this rule is a super-rule, and 'list' is the head of + * a list of subrules. A super-rule with no wildcards (where + * cr.wc.wildcards is 0) will never have any subrules. */ struct rule *super; struct list list; @@ -165,7 +161,7 @@ static struct rule *rule_create(struct rule *super, const union ofp_action *, size_t n_actions, uint16_t idle_timeout, uint16_t hard_timeout); static void rule_free(struct rule *); -static void rule_destroy(struct rule *); +static void rule_destroy(struct ofproto *, struct rule *); static struct rule *rule_from_cls_rule(const struct cls_rule *); static bool rule_make_actions(struct ofproto *, struct rule *, const struct ofpbuf *packet); @@ -968,7 +964,7 @@ ofproto_add_flow(struct ofproto *p, displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr)); if (displaced_rule) { - rule_destroy(displaced_rule); + rule_destroy(p, displaced_rule); } if (!wildcards) { @@ -1002,7 +998,7 @@ ofproto_delete_flow(struct ofproto *ofproto, const flow_t *flow, ofproto->need_revalidate = true; } classifier_remove(&ofproto->cls, &rule->cr); - rule_destroy(rule); + rule_destroy(ofproto, rule); } } @@ -1371,15 +1367,23 @@ rule_free(struct rule *rule) free(rule); } +/* Destroys 'rule'. If 'rule' is a subrule, also removes it from its + * super-rule's list of subrules. If 'rule' is a super-rule, also iterates + * through all of its subrules and revalidates them, destroying any that no + * longer has a super-rule (which is probably all of them). + * + * Before calling this function, the caller must make have removed 'rule' from + * the classifier. If 'rule' is an exact-match rule, the caller is also + * responsible for ensuring that it has been uninstalled from the datapath. */ static void -rule_destroy(struct rule *rule) +rule_destroy(struct ofproto *ofproto, struct rule *rule) { if (!rule->super) { - struct rule *subrule; - LIST_FOR_EACH (subrule, struct rule, list, &rule->list) { - subrule->super = UNKNOWN_SUPER; + struct rule *subrule, *next; + LIST_FOR_EACH_SAFE (subrule, next, struct rule, list, &rule->list) { + revalidate_rule(ofproto, subrule); } - } else if (rule->super != UNKNOWN_SUPER) { + } else { list_remove(&rule->list); } rule_free(rule); @@ -1412,7 +1416,6 @@ rule_make_actions(struct ofproto *p, struct rule *rule, struct odp_actions a; size_t actions_len; - assert(rule->super != UNKNOWN_SUPER); assert(!rule->cr.wc.wildcards); super = rule->super ? rule->super : rule; @@ -1478,8 +1481,7 @@ rule_uninstall(struct ofproto *p, struct rule *rule) odp_flow.actions = NULL; odp_flow.n_actions = 0; if (!dpif_flow_del(&p->dpif, &odp_flow)) { - update_stats(rule->super && rule->super != UNKNOWN_SUPER - ? rule->super : rule, &odp_flow.stats); + update_stats(rule->super ? rule->super : rule, &odp_flow.stats); } rule->installed = false; @@ -1706,7 +1708,7 @@ lookup_valid_rule(struct ofproto *ofproto, const flow_t *flow) * revalidation. If it is not valid, don't return it. */ if (rule && rule->super - && (ofproto->need_revalidate || rule->super == UNKNOWN_SUPER) + && ofproto->need_revalidate && !revalidate_rule(ofproto, rule)) { return NULL; } @@ -2453,9 +2455,10 @@ add_flow(struct ofproto *p, struct ofconn *ofconn, * the subrule that got actions from 'displaced_rule'. We could * add more data to track such situations, but we have no evidence * that the optimization would be worthwhile performance-wise - * anyhow. + * anyhow. So we just let rule_destroy() check each subrule and + * transfer or destroy them as necessary. */ - rule_destroy(displaced_rule); + rule_destroy(p, displaced_rule); } /* We might need to change the rules for arbitrary subrules. */ @@ -2466,11 +2469,10 @@ add_flow(struct ofproto *p, struct ofconn *ofconn, rule_make_actions(p, rule, packet); rule_install(p, rule, &stats); if (displaced_rule) { - if (displaced_rule->super && - displaced_rule->super != UNKNOWN_SUPER) { + if (displaced_rule->super) { update_stats(displaced_rule->super, &stats); } - rule_destroy(displaced_rule); + rule_destroy(p, displaced_rule); } } ofpbuf_delete(packet); @@ -2493,7 +2495,7 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm, rule_uninstall(p, rule); } classifier_remove(&p->cls, &rule->cr); - rule_destroy(rule); + rule_destroy(p, rule); } else { free(rule->actions); rule->actions = xmemdup(ofm->actions, @@ -2574,6 +2576,7 @@ modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm, cbdata.command = command; cls_rule_from_match(&target, &ofm->match, 0); + classifier_for_each_match(&p->cls, &target, CLS_INC_ALL, modify_flows_cb, &cbdata); return 0; @@ -2834,14 +2837,14 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet) /* We just replaced a real rule with a subrule. We shouldn't * have done that. Put old_sr back and destroy the subrule. */ cls_rule_replace(&p->cls, &subrule->cr, &old_sr->cr); - rule_destroy(subrule); + rule_destroy(p, subrule); subrule = old_sr; } else { /* XXX this seems wrong: why are we accounting the old rule's * traffic to the new one? */ subrule->packet_count += old_sr->packet_count; subrule->byte_count += old_sr->byte_count; - rule_destroy(old_sr); + rule_destroy(p, old_sr); rule_make_actions(p, subrule, packet); } } else { @@ -2897,12 +2900,10 @@ revalidate_rule(struct ofproto *p, struct rule *rule) if (!super) { rule_uninstall(p, rule); classifier_remove(&p->cls, &rule->cr); - rule_destroy(rule); + rule_destroy(p, rule); return false; } else if (super != rule->super) { - if (rule->super != UNKNOWN_SUPER) { - list_remove(&rule->list); - } + list_remove(&rule->list); list_push_back(&super->list, &rule->list); rule->super = super; rule->hard_timeout = super->hard_timeout; @@ -2970,7 +2971,7 @@ uninstall_idle_flow(struct ofproto *ofproto, struct rule *rule) rule_uninstall(ofproto, rule); if (rule->super) { classifier_remove(&ofproto->cls, &rule->cr); - rule_destroy(rule); + rule_destroy(ofproto, rule); } } @@ -3014,7 +3015,7 @@ expire_rule(struct cls_rule *cls_rule, void *p_) struct rule, list, &rule->list) { rule_uninstall(p, subrule); classifier_remove(&p->cls, &subrule->cr); - rule_destroy(subrule); + rule_destroy(p, subrule); } } else { rule_uninstall(p, rule); @@ -3025,7 +3026,7 @@ expire_rule(struct cls_rule *cls_rule, void *p_) (now >= hard_expire ? OFPER_HARD_TIMEOUT : OFPER_IDLE_TIMEOUT)); classifier_remove(&p->cls, &rule->cr); - rule_destroy(rule); + rule_destroy(p, rule); } static void -- 2.30.2