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

[Ben Pfaff]

On Tue, Oct 12, 2010 at 03:34:05PM -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 its "children"
> statistics as well.

Thanks for writing this up.  It should make debugging with resubmits
much easier.

All three instances of "it's" in this patch should be "its".

I see several instances of list_init(&rule->rs_node) in this patch.  Are
they useful?  I think that they can all be removed, because rs_node is
only used when the rule is actually on a list (when rs_parent != NULL).

I think that multiple rules can submit into a single rule, e.g. both A
and B could submit into C.  I don't think that case is handled, because
it would require C to have two parents.  Is there some reason that this
case can't happen, or some other reason it should work OK anyhow?

Why does del_resubmit_rule() set ofproto->need_revalidate to true?  As
far as I can tell this would only be necessary if there were an existing
bug that it was fixing, but I don't know of one.

Could the 'rule' parameter to add_resubmit_rule() be renamed 'child'?
It would clarify matters for me a bit.

The adjustments to packet_count and byte_count in add_resubmit_rule()
make it look like parent's packet_count and byte_count could wrap around
to UINT64_MAX.  That seems bad.  Can it cause anything bad to happen?

In struct action_xlate_ctx, I think that ctx->rule is going to be the
"child" in any parent-child relationship that is set up by resubmit, and
I don't think that it is used elsewhere.  Could we name it "child" or
something like that, and change the comment appropriately?  While
reading this, I kept getting confused on which was the parent and which
was the child, and this might help.

In rule_make_actions(), why remove "const" from the 'super' local
variable?

In xlate_table_action(), the removal of the "rule = rule->super;" for
subrules looks wrong.  It is needed so that, if a subrule is found in
the search, we use the super-rule's OpenFlow actions as the input to
translation.  That is in turn needed because subrules don't have
OpenFlow actions, only their super-rules do.  (The parent-child
relationship is different; I agree that it makes sense to include
subrules in that relationship.)

Thanks,

Ben.