[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