From: Ben Pfaff Date: Wed, 29 Apr 2009 22:43:54 +0000 (-0700) Subject: secchan: Clean up and simplify handle_odp_msg(). X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ed39db7c9354139b57a76306060d5ac915208c97;p=openvswitch secchan: Clean up and simplify handle_odp_msg(). --- diff --git a/lib/classifier.c b/lib/classifier.c index 25516622..7ef4119f 100644 --- a/lib/classifier.c +++ b/lib/classifier.c @@ -233,6 +233,18 @@ classifier_insert(struct classifier *cls, struct cls_rule *rule) return old; } +/* Inserts 'rule' into 'cls'. Transfers ownership of 'rule' to 'cls'. + * + * 'rule' must be an exact-match rule (rule->wc.wildcards must be 0) and 'cls' + * must not contain any rule with an identical key. */ +void +classifier_insert_exact(struct classifier *cls, struct cls_rule *rule) +{ + hmap_insert(&cls->exact_table, &rule->node.hmap, + flow_hash(&rule->flow, 0)); + cls->n_rules++; +} + /* Removes 'rule' from 'cls'. It is caller's responsibility to free 'rule', if * this is desirable. */ void diff --git a/lib/classifier.h b/lib/classifier.h index dd2fb525..4f547f80 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -138,6 +138,7 @@ bool classifier_is_empty(const struct classifier *); int classifier_count(const struct classifier *); int classifier_count_exact(const struct classifier *); struct cls_rule *classifier_insert(struct classifier *, struct cls_rule *); +void classifier_insert_exact(struct classifier *, struct cls_rule *); void classifier_remove(struct classifier *, struct cls_rule *); struct cls_rule *classifier_lookup(const struct classifier *, const flow_t *); struct cls_rule *classifier_lookup_wild(const struct classifier *, diff --git a/secchan/ofproto.c b/secchan/ofproto.c index 12b7bc12..6ea28c7f 100644 --- a/secchan/ofproto.c +++ b/secchan/ofproto.c @@ -1430,6 +1430,20 @@ rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet) } } +static struct rule * +rule_create_subrule(struct ofproto *ofproto, struct rule *rule, + const flow_t *flow) +{ + struct rule *subrule = rule_create(rule, NULL, 0, + rule->idle_timeout, rule->hard_timeout); + cls_rule_from_flow(&subrule->cr, flow, 0, + (rule->cr.priority <= UINT16_MAX ? UINT16_MAX + : rule->cr.priority)); + classifier_insert_exact(&ofproto->cls, &subrule->cr); + + return subrule; +} + static void rule_remove(struct ofproto *ofproto, struct rule *rule) { @@ -2797,7 +2811,7 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet) { struct odp_msg *msg = packet->data; uint16_t in_port = odp_port_to_ofp_port(msg->port); - struct rule *rule, *subrule; + struct rule *rule; struct ofpbuf payload; flow_t flow; @@ -2831,56 +2845,33 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet) } if (rule->cr.wc.wildcards) { - struct rule *old_sr; - - subrule = rule_create(rule, NULL, 0, - rule->idle_timeout, rule->hard_timeout); - cls_rule_from_flow(&subrule->cr, &flow, 0, - (rule->cr.priority <= UINT16_MAX ? UINT16_MAX - : rule->cr.priority)); - - /* Insert 'subrule' into the classifier and compose actions for it. */ - old_sr = rule_from_cls_rule(classifier_insert(&p->cls, &subrule->cr)); - if (old_sr) { - if (!old_sr->super) { - /* 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(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(p, old_sr); - rule_make_actions(p, subrule, packet); - } - } else { - rule_make_actions(p, subrule, packet); - } - - /* Install flow entry into datapath. */ - rule_install(p, subrule, NULL); + rule = rule_create_subrule(p, rule, &flow); + rule_make_actions(p, rule, packet); } else { - subrule = rule; if (!rule->may_install) { /* The rule is not installable, that is, we need to process every - * packet, so do that here. */ - rule_make_actions(p, subrule, packet); - rule_install(p, subrule, NULL); + * packet, so process the current packet and set its actions into + * 'subrule'. */ + rule_make_actions(p, rule, packet); } else { - /* A flow got dropped due to a hash collision, or the packet was - * buffered before we processed another packet from the same - * flow. */ + /* XXX revalidate rule if it needs it */ } } - /* Execute subrule on packet. */ - if (!dpif_execute(&p->dpif, msg->port, subrule->odp_actions, - subrule->n_odp_actions, &payload)) { - subrule->packet_count++; - subrule->byte_count += payload.size; + /* Invariant: 'rule' is an exact-match rule. */ + + /* XXX should forcibly install the rule here, since it's possible that it + * got deleted externally (e.g. "dpctl dp-del-flows") and unless we + * re-install it here we'll end up processing every packet by hand. But + * the datapath doesn't provide a good interface for that. */ + rule_install(p, rule, NULL); + + /* Execute rule on packet. */ + if (!dpif_execute(&p->dpif, msg->port, rule->odp_actions, + rule->n_odp_actions, &payload)) { + /* XXX use flow_extract_stats(). */ + rule->packet_count++; + rule->byte_count += payload.size; } ofpbuf_delete(packet); } diff --git a/tests/test-classifier.c b/tests/test-classifier.c index b281134e..a47e8380 100644 --- a/tests/test-classifier.c +++ b/tests/test-classifier.c @@ -520,7 +520,11 @@ test_single_rule(void) tcls_init(&tcls); tcls_rule = tcls_insert(&tcls, rule); - assert(!classifier_insert(&cls, &rule->cls_rule)); + if (wc_fields) { + assert(!classifier_insert(&cls, &rule->cls_rule)); + } else { + classifier_insert_exact(&cls, &rule->cls_rule); + } check_tables(&cls, 1, 1, 1); compare_classifiers(&cls, &tcls);