[ovs-dev] [PATCH 1/2] ofproto: Resubmit packet and byte count statistics.
Ethan Jackson
ethan at nicira.com
Thu Oct 14 18:15:20 UTC 2010
Oops sorry I forgot to respond to this. I did verify it with that
exact case and it works as expected. I think the key idea here is
that wildcarded match rules are split into exact match subrules before
installed into the datapath. Thus rule A in that case would be split
into two exact match rules. A particular exact match rule can only
resubmit into one rule.
Ethan Jackson
On Tue, Oct 12, 2010 at 9:13 PM, Ethan Jackson <ethan at nicira.com> wrote:
> 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