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

Ethan Jackson ethan at nicira.com
Wed Oct 13 04:13:25 UTC 2010


In this case rule A would be split into two sub-rules one which
resubmits into rule B (and matches on mac1) and another which
resubmits into rule C (and matches on mac2).  I'll go ahead and verify
that it behaves as expected when I get in tomorrow, but I'm pretty
sure I tested cases similar to this.

Ethan

On Tue, Oct 12, 2010 at 7:33 PM, Reid Price <reid at nicira.com> wrote:
> Hmm, I probably should've given a more illustrative example then.  What
> about this?
>
> A => in_port=1, priority=300, actions=resubmit:5
> B => in_port=5, priority=100, dl_dst=<MAC1>, actions=1
> C => in_port=5, priority=100, dl_dst=<MAC2>, actions=2
>
> The issue I'm failing to describe properly is that A does not
> deterministically
> resubmit into a single rule B or C, but conditionally into B, C, or neither.
>
> If you get 3 packets each with MAC1, MAC2, and MAC3 on port 1, I would
> expect
> the correct counts to be A -> 9, B -> 3, C -> 3.  Is that the case
> currently?
>
>   -Reid
>
> On Tue, Oct 12, 2010 at 5:48 PM, Ethan Jackson <ethan at nicira.com> wrote:
>>
>> This patch has been superseded by "[PATCH 1/2] ofproto: Resubmit
>> packet and byte count statistics[]" which was submitted earlier today.
>>  That said, the patch (at least the newest version) behaves as you
>> expect it should.  Suppose you have a resubmit tree rooted at rule_a.
>> This tree does not contain *all* rules that could possibly resubmit
>> into it.  Instead, it contains the rules that resubmit into it *now*.
>> So with your example:
>>
>> A => in_port=1, priority=300, actions=resubmit:5
>> B => in_port=5, priority=200, actions=1
>> C => in_port=5, priority=100, actions=2
>>
>> Suppose a packet comes in at port 1.  A resubmit tree will be built
>> with B at the root and A as it's child.  C is not in the resubmit tree
>> because nothing resubmits into it.  Thus any packets going to port 1
>> will only be credited to A and B.  Now suppose we delete flow B and a
>> new packet comes in port A.  A wil recalculate it's actions and
>> discover that it resubmits into C.  Thus a new flow tree will be build
>> rooted at C with A as a child.  Any new packets flowing through port
>> one will be credited to A and C.
>>
>> This stuff is surprisingly subtle/complex so it's completely possible
>> that I screwed something up.  But I tested the example you showed by
>> hand and it worked as expected for me (at least on the lastest version
>> of the patch).
>>
>>
>>
>>
>> On Tue, Oct 12, 2010 at 5:08 PM, Reid Price <reid at nicira.com> wrote:
>> > I am hopefully missing something obvious.  It seems as though this logic
>> > assumes that
>> > multiple rules may resubmit to a particular rule (i.e.:  a parent has
>> > multiple children), but
>> > that a resubmit cannot terminate in multiple rules (a child has multiple
>> > parents).  With the
>> > following rules, or perhaps a non-contrived one that could actually be
>> > reached:
>> >
>> > in_port=1, priority=300, actions=resubmit:5
>> > in_port=5, priority=200, actions=1
>> > in_port=5, priority=100, actions=2
>> >
>> > If 10 packets came in port 1, I would expect the counts to be:
>> >
>> > in_port=1, priority=300, actions=resubmit:5   [10]
>> > in_port=5, priority=200, actions=1  [0 + 10 resubmit = 10]
>> > in_port=5, priority=100, actions=2  [0]
>> >
>> >    not
>> >
>> > in_port=1, priority=300, actions=resubmit:5   [10]
>> > in_port=5, priority=200, actions=1  [0 + 10 resubmit = 10]
>> > in_port=5, priority=100, actions=2  [0 + 10 resubmit = 10]
>> >
>> > I am only looking at the diff, not the full source, but based upon the
>> > call
>> > to query_stats, I
>> > don't see how you distinguish between the two in_port=1 rule for the two
>> > in_port=5 rules:
>> >
>> >   query_stats(p, rs_rule, &rs_packet_count, &rs_byte_count);
>> >
>> > I apologize if this is untrue, or already fixed with what Ben pointed
>> > out in
>> > PATCH 1/1 of
>> > this same name  =)
>> >
>> >   -Reid
>> >
>> > On Tue, Oct 12, 2010 at 3:34 PM, Ethan Jackson <ethan at nicira.com> 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.
>> >> ---
>> >>  ofproto/ofproto.c |  132
>> >> ++++++++++++++++++++++++++++++++++++++++++++++-------
>> >>  1 files changed, 115 insertions(+), 17 deletions(-)
>> >>
>> >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> >> index e25ce28..401a11a 100644
>> >> --- a/ofproto/ofproto.c
>> >> +++ b/ofproto/ofproto.c
>> >> @@ -80,12 +80,6 @@ struct ofport {
>> >>  static void ofport_free(struct ofport *);
>> >>  static void hton_ofp_phy_port(struct ofp_phy_port *);
>> >>
>> >> -static int xlate_actions(const union ofp_action *in, size_t n_in,
>> >> -                         const struct flow *, struct ofproto *,
>> >> -                         const struct ofpbuf *packet,
>> >> -                         struct odp_actions *out, tag_type *tags,
>> >> -                         bool *may_set_up_flow, uint16_t
>> >> *nf_output_iface);
>> >> -
>> >>  struct rule {
>> >>     struct cls_rule cr;
>> >>
>> >> @@ -131,8 +125,27 @@ struct rule {
>> >>                                  * be reassessed for every packet. */
>> >>     int n_odp_actions;
>> >>     union odp_action *odp_actions;
>> >> +
>> >> +    /* 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 in its rs_parent. */
>> >> +    struct rule *rs_parent;
>> >> +    struct list rs_node;
>> >> +    struct list rs_children;
>> >>  };
>> >>
>> >> +static int xlate_actions(const union ofp_action *in, size_t n_in,
>> >> +                         struct rule *, const struct flow *,
>> >> +                         struct ofproto *, const struct ofpbuf
>> >> *packet,
>> >> +                         struct odp_actions *out, tag_type *tags,
>> >> +                         bool *may_set_up_flow, uint16_t
>> >> *nf_output_iface);
>> >> +
>> >> +static void query_stats(struct ofproto *, struct rule *,
>> >> +                        uint64_t *packet_countp, uint64_t
>> >> *byte_countp);
>> >> +
>> >>  static inline bool
>> >>  rule_is_hidden(const struct rule *rule)
>> >>  {
>> >> @@ -170,6 +183,9 @@ static void rule_uninstall(struct ofproto *, struct
>> >> rule *);
>> >>  static void rule_post_uninstall(struct ofproto *, struct rule *);
>> >>  static void send_flow_removed(struct ofproto *p, struct rule *rule,
>> >>                               long long int now, uint8_t reason);
>> >> +static void add_resubmit_rule(struct ofproto *,
>> >> +                              struct rule *parent, struct rule *);
>> >> +static void del_resubmit_rule(struct ofproto *, struct rule *);
>> >>
>> >>  /* ofproto supports two kinds of OpenFlow connections:
>> >>  *
>> >> @@ -1285,7 +1301,8 @@ ofproto_send_packet(struct ofproto *p, const
>> >> struct
>> >> flow *flow,
>> >>     struct odp_actions odp_actions;
>> >>     int error;
>> >>
>> >> -    error = xlate_actions(actions, n_actions, flow, p, packet,
>> >> &odp_actions,
>> >> +    error = xlate_actions(actions, n_actions, NULL,
>> >> +                          flow, p, packet, &odp_actions,
>> >>                           NULL, NULL, NULL);
>> >>     if (error) {
>> >>         return error;
>> >> @@ -1854,6 +1871,9 @@ rule_create(struct ofproto *ofproto, struct rule
>> >> *super,
>> >>         rule->n_actions = n_actions;
>> >>         rule->actions = xmemdup(actions, n_actions * sizeof *actions);
>> >>     }
>> >> +
>> >> +    list_init(&rule->rs_node);
>> >> +    list_init(&rule->rs_children);
>> >>     netflow_flow_clear(&rule->nf_flow);
>> >>     netflow_flow_update_time(ofproto->netflow, &rule->nf_flow,
>> >> rule->created);
>> >>
>> >> @@ -1885,6 +1905,8 @@ rule_free(struct rule *rule)
>> >>  static void
>> >>  rule_destroy(struct ofproto *ofproto, struct rule *rule)
>> >>  {
>> >> +    del_resubmit_rule(ofproto, rule);
>> >> +
>> >>     if (!rule->super) {
>> >>         struct rule *subrule, *next;
>> >>         LIST_FOR_EACH_SAFE (subrule, next, list, &rule->list) {
>> >> @@ -1896,6 +1918,61 @@ rule_destroy(struct ofproto *ofproto, struct
>> >> rule
>> >> *rule)
>> >>     rule_free(rule);
>> >>  }
>> >>
>> >> +/* Deletes 'rule' from the resubmit tree it may or may not be in.
>> >>  This
>> >> involves
>> >> + * removing 'rule' from it's parent, updating it's parent's count
>> >> statistics,
>> >> + * and updating it's children */
>> >> +static void
>> >> +del_resubmit_rule(struct ofproto *ofproto, struct rule *rule)
>> >> +{
>> >> +
>> >> +    if (rule->rs_parent) {
>> >> +        uint64_t packet_count, byte_count;
>> >> +
>> >> +        query_stats(ofproto, rule, &packet_count, &byte_count);
>> >> +
>> >> +        assert(!list_is_empty(&rule->rs_node));
>> >> +        list_remove(&rule->rs_node);
>> >> +        list_init(&rule->rs_node);
>> >> +
>> >> +        rule->rs_parent->packet_count += packet_count;
>> >> +        rule->rs_parent->byte_count   += byte_count;
>> >> +        rule->rs_parent                = NULL;
>> >> +    }
>> >> +
>> >> +    if (!list_is_empty(&rule->rs_children)) {
>> >> +        struct rule *child, *child_next;
>> >> +        ofproto->need_revalidate = true;
>> >> +
>> >> +        LIST_FOR_EACH_SAFE(child, child_next, rs_node,
>> >> &rule->rs_children) {
>> >> +            child->rs_parent = NULL;
>> >> +            list_remove(&child->rs_node);
>> >> +            list_init(&child->rs_node);
>> >> +        }
>> >> +    }
>> >> +}
>> >> +
>> >> +/* Add 'rule' to the resubmit tree of 'parent'. */
>> >> +static void
>> >> +add_resubmit_rule(struct ofproto *ofproto,
>> >> +                  struct rule *parent, struct rule *rule)
>> >> +{
>> >> +    uint64_t packet_count, byte_count;
>> >> +
>> >> +    if (rule->rs_parent == parent) {
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    assert(parent != rule);
>> >> +
>> >> +    del_resubmit_rule(ofproto, rule);
>> >> +    query_stats(ofproto, rule, &packet_count, &byte_count);
>> >> +    parent->packet_count -= packet_count;
>> >> +    parent->byte_count   -= byte_count;
>> >> +
>> >> +    rule->rs_parent = parent;
>> >> +    list_push_back(&parent->rs_children, &rule->rs_node);
>> >> +}
>> >> +
>> >>  static bool
>> >>  rule_has_out_port(const struct rule *rule, uint16_t out_port)
>> >>  {
>> >> @@ -1984,7 +2061,8 @@ rule_execute(struct ofproto *ofproto, struct rule
>> >> *rule,
>> >>      * scenario. */
>> >>     if (rule->cr.wc.wildcards || !flow_equal(flow, &rule->cr.flow)) {
>> >>         struct rule *super = rule->super ? rule->super : rule;
>> >> -        if (xlate_actions(super->actions, super->n_actions, flow,
>> >> ofproto,
>> >> +        if (xlate_actions(super->actions, super->n_actions,
>> >> +                          rule, flow, ofproto,
>> >>                           packet, &a, NULL, 0, NULL)) {
>> >>             ofpbuf_delete(packet);
>> >>             return;
>> >> @@ -2075,6 +2153,8 @@ rule_create_subrule(struct ofproto *ofproto,
>> >> struct
>> >> rule *rule,
>> >>  static void
>> >>  rule_remove(struct ofproto *ofproto, struct rule *rule)
>> >>  {
>> >> +    del_resubmit_rule(ofproto, rule);
>> >> +
>> >>     if (rule->cr.wc.wildcards) {
>> >>         COVERAGE_INC(ofproto_del_wc_flow);
>> >>         ofproto->need_revalidate = true;
>> >> @@ -2090,7 +2170,7 @@ static bool
>> >>  rule_make_actions(struct ofproto *p, struct rule *rule,
>> >>                   const struct ofpbuf *packet)
>> >>  {
>> >> -    const struct rule *super;
>> >> +    struct rule *super;
>> >>     struct odp_actions a;
>> >>     size_t actions_len;
>> >>
>> >> @@ -2098,7 +2178,7 @@ rule_make_actions(struct ofproto *p, struct rule
>> >> *rule,
>> >>
>> >>     super = rule->super ? rule->super : rule;
>> >>     rule->tags = 0;
>> >> -    xlate_actions(super->actions, super->n_actions, &rule->cr.flow, p,
>> >> +    xlate_actions(super->actions, super->n_actions, rule,
>> >> &rule->cr.flow,
>> >> p,
>> >>                   packet, &a, &rule->tags, &rule->may_install,
>> >>                   &rule->nf_flow.output_iface);
>> >>
>> >> @@ -2439,6 +2519,7 @@ add_controller_action(struct odp_actions
>> >> *actions,
>> >> uint16_t max_len)
>> >>
>> >>  struct action_xlate_ctx {
>> >>     /* Input. */
>> >> +    struct rule *rule;          /* Rule to which these actions
>> >> correspond. */
>> >>     struct flow flow;           /* Flow to which these actions
>> >> correspond.
>> >> */
>> >>     int recurse;                /* Recursion level, via
>> >> xlate_table_action. */
>> >>     struct ofproto *ofproto;
>> >> @@ -2518,13 +2599,19 @@ xlate_table_action(struct action_xlate_ctx
>> >> *ctx,
>> >> uint16_t in_port)
>> >>         ctx->flow.in_port = old_in_port;
>> >>
>> >>         if (rule) {
>> >> -            if (rule->super) {
>> >> -                rule = rule->super;
>> >> -            }
>> >> +            struct rule *ctx_rule;
>> >>
>> >> +            ctx_rule = ctx->rule;
>> >> +            ctx->rule = ctx->rule ? rule : NULL;
>> >>             ctx->recurse++;
>> >>             do_xlate_actions(rule->actions, rule->n_actions, ctx);
>> >>             ctx->recurse--;
>> >> +            ctx->rule = ctx_rule;
>> >> +
>> >> +            if (ctx->rule) {
>> >> +                add_resubmit_rule(ctx->ofproto, rule, ctx->rule);
>> >> +            }
>> >> +
>> >>         }
>> >>     } else {
>> >>         struct vlog_rate_limit recurse_rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> >> @@ -2835,8 +2922,8 @@ do_xlate_actions(const union ofp_action *in,
>> >> size_t
>> >> n_in,
>> >>
>> >>  static int
>> >>  xlate_actions(const union ofp_action *in, size_t n_in,
>> >> -              const struct flow *flow, struct ofproto *ofproto,
>> >> -              const struct ofpbuf *packet,
>> >> +              struct rule *rule, const struct flow *flow,
>> >> +              struct ofproto *ofproto, const struct ofpbuf *packet,
>> >>               struct odp_actions *out, tag_type *tags, bool
>> >> *may_set_up_flow,
>> >>               uint16_t *nf_output_iface)
>> >>  {
>> >> @@ -2844,6 +2931,7 @@ xlate_actions(const union ofp_action *in, size_t
>> >> n_in,
>> >>     struct action_xlate_ctx ctx;
>> >>     COVERAGE_INC(ofproto_ofp2odp);
>> >>     odp_actions_init(out);
>> >> +    ctx.rule = rule;
>> >>     ctx.flow = *flow;
>> >>     ctx.recurse = 0;
>> >>     ctx.ofproto = ofproto;
>> >> @@ -2935,7 +3023,7 @@ handle_packet_out(struct ofproto *p, struct
>> >> ofconn
>> >> *ofconn,
>> >>
>> >>     flow_extract(&payload, 0,
>> >> ofp_port_to_odp_port(ntohs(opo->in_port)),
>> >> &flow);
>> >>     error = xlate_actions((const union ofp_action *) opo->actions,
>> >> n_actions,
>> >> -                          &flow, p, &payload, &actions, NULL, NULL,
>> >> NULL);
>> >> +                          NULL, &flow, p, &payload, &actions, NULL,
>> >> NULL,
>> >> NULL);
>> >>     if (error) {
>> >>         return error;
>> >>     }
>> >> @@ -3182,7 +3270,7 @@ query_stats(struct ofproto *p, struct rule *rule,
>> >>             uint64_t *packet_countp, uint64_t *byte_countp)
>> >>  {
>> >>     uint64_t packet_count, byte_count;
>> >> -    struct rule *subrule;
>> >> +    struct rule *subrule, *rs_rule;
>> >>     struct odp_flow *odp_flows;
>> >>     size_t n_odp_flows;
>> >>
>> >> @@ -3223,6 +3311,16 @@ query_stats(struct ofproto *p, struct rule
>> >> *rule,
>> >>     }
>> >>     free(odp_flows);
>> >>
>> >> +    LIST_FOR_EACH (rs_rule, rs_node, &rule->rs_children) {
>> >> +        uint64_t rs_packet_count, rs_byte_count;
>> >> +
>> >> +        assert(rs_rule->rs_parent == rule);
>> >> +
>> >> +        query_stats(p, rs_rule, &rs_packet_count, &rs_byte_count);
>> >> +        packet_count += rs_packet_count;
>> >> +        byte_count   += rs_byte_count;
>> >> +    }
>> >> +
>> >>     /* Return the stats to the caller. */
>> >>     *packet_countp = packet_count;
>> >>     *byte_countp = byte_count;
>> >> --
>> >> 1.7.3.1
>> >>
>> >>
>> >> _______________________________________________
>> >> dev mailing list
>> >> dev at openvswitch.org
>> >> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
>> >
>> >
>
>




More information about the dev mailing list