[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 22:57:23 UTC 2010


Nifty. Ill get through the rest of the patches later today/tomorrow.

Ethan

On Fri, Nov 12, 2010 at 2:50 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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