[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