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

Ethan Jackson ethan at nicira.com
Fri Nov 12 19:28:01 UTC 2010


This is a major improvement in the openflow code.  It's definitely
going to be easier to reason about in the future.


> +    /* 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.

> @@ -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?  It seems like it
would be cleaner for rule_insert to simply insert the rule and let the
caller send the packet if they want to (there are only two callers,
one does and one doesn't).  It was like this before the patch so I
think it's fine to leave it, but perhaps in a future patch we should
change it.


> +static bool
> +facet_revalidate(struct ofproto *ofproto, struct facet *facet)
> +{
> +    struct rule *rule;
>
> -        netflow_flow_clear(&rule->nf_flow);
> +    COVERAGE_INC(facet_revalidate);
> +    rule = rule_lookup(ofproto, &facet->flow);
> +    if (!rule) {
> +        facet_remove(ofproto, facet);
> +        return false;
> +    }
> +
> +    if (rule != facet->rule) {
> +        COVERAGE_INC(facet_changed_rule);
> +        list_remove(&facet->list_node);
> +        list_push_back(&rule->facets, &facet->list_node);
> +        facet->rule = rule;
> +        facet->used = rule->created;
>     }

Should we be crediting facet's statistics to it's former rule before
moving it?  Not super sure, just a thought.

Ethan




More information about the dev mailing list