[ovs-dev] [PATCH 1/2] ofproto: Resubmit packet and byte count statistics.
Ethan Jackson
ethan at nicira.com
Wed Oct 13 00:48:38 UTC 2010
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