[ovs-dev] [PATCH 1/1] ofproto: Resubmit packet and byte count statistics.

Ben Pfaff blp at nicira.com
Tue Oct 12 17:54:23 UTC 2010


On Fri, Oct 08, 2010 at 02:01:11PM -0700, Ethan Jackson wrote:
> This patch maintains packet and byte count statistics when
> resubmits are involved.  For a particular rule A, it builds a tree
> rooted at A of all rules which resubmit into it.  Thus when
> statistics are queried for A, it can incorporate it's "children"

"its" not "it's" here

> statistics as well.
> ---

...

> +    /* Resubmit Tree.
> +     *
> +     * Rules maintain a "resubmit tree" used to keep track of packet and byte
> +     * count statistics.  Suppose rule_a resubmits into rule_b.  In this case,
> +     * rule_b will maintain the rule_a rs_node in its rs_children list.  In
> +     * addition rule_a will store rule_b its rs_parent. */

...store rule_b "in" its rs_parent?

> +    struct rule * rs_parent;

Please write pointers as "type *name" not "type * name".

> +    struct list rs_node;
> +    struct list rs_children;
>  };
>  
> +static int xlate_actions(const union ofp_action *in, size_t n_in,
> +                         struct rule * rule, const flow_t *flow,

"struct rule *rule"

> +                         struct ofproto *ofproto, const struct ofpbuf *packet,
> +                         struct odp_actions *out, tag_type *tags,
> +                         bool *may_set_up_flow, uint16_t *nf_output_iface);

When writing function prototypes, please leave out parameter names when
they add no information.  For example, instead of "struct rule *rule",
write "struct rule *", and instead of "struct ofproto *ofproto", just
write "struct ofproto *".  But the parameter name in "uint16_t
*nf_output_iface" gives the reader a hint about the purpose of the
parameter, so leave it in there.

> +static void query_stats(struct ofproto *p, struct rule *rule,
> +                        uint64_t *packet_countp, uint64_t *byte_countp);
> +
>  static inline bool
>  rule_is_hidden(const struct rule *rule)
>  {

At this point I stopped reviewing details, because I started thinking
about the data structure in general.  Let's talk about that first, and
then I'll go back to the details after I understand the high points.

I don't understand why this is done in terms of super-rules.  It seems
to me that that approach must credit the wrong rules in some cases.
Take an example.  Suppose that we have an OpenFlow flow table with three
rules:

        Rule A: low priority, matches every packet, resubmits with
        in_port=5.

        Rule B: high priority, matches TCP packets sent to port 80 that
        have in_port=5.

        Rule C: high priority, matches TCP packets sent to port 443 that
        have in_port=5.

Suppose a packet arrives with in_port=1, tp_dst=80.  It hits A and
resubmits to B.  This sets up a tree with B as the root and A as its
child, as I understand it.  Then another packet arrives with in_port=1,
tp_dst=443.  It hits A and resubmits to C.  This tries to set up a tree
with C as root and A as its children.  Doesn't this fail the assertion
here in add_resubmit_rule():

    if (rule->rs_parent) {
        assert(rule->rs_parent == parent);
        return;
    }




More information about the dev mailing list