ofproto: Move all statistics accounting into xlate_actions().
authorEthan Jackson <ethan@nicira.com>
Wed, 13 Nov 2013 02:18:01 +0000 (18:18 -0800)
committerEthan Jackson <ethan@nicira.com>
Fri, 13 Dec 2013 04:21:11 +0000 (20:21 -0800)
This patch moves statistics accounting for netflow, bonding, netdev,
and mirroring inside xlate_actions().  Moving all statistics into one
place makes it very difficult to mess up.

Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
ofproto/ofproto-dpif-xlate.c
ofproto/ofproto-dpif-xlate.h
ofproto/ofproto-dpif.c

index cea4658..402f54b 100644 (file)
@@ -219,10 +219,10 @@ static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
                              struct xlate_ctx *);
 static void xlate_actions__(struct xlate_in *, struct xlate_out *)
     OVS_REQ_RDLOCK(xlate_rwlock);
-static void xlate_normal(struct xlate_ctx *);
-static void xlate_report(struct xlate_ctx *, const char *);
-static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
-                               uint8_t table_id, bool may_packet_in);
+    static void xlate_normal(struct xlate_ctx *);
+    static void xlate_report(struct xlate_ctx *, const char *);
+    static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
+                                   uint8_t table_id, bool may_packet_in);
 static bool input_vid_is_valid(uint16_t vid, struct xbundle *, bool warn);
 static uint16_t input_vid_to_vlan(const struct xbundle *, uint16_t vid);
 static void output_normal(struct xlate_ctx *, const struct xbundle *,
@@ -559,8 +559,8 @@ xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
     }
 
     xport = xport_lookup(tnl_port_should_receive(flow)
-            ? tnl_port_receive(flow)
-            : odp_port_to_ofport(backer, flow->in_port.odp_port));
+                         ? tnl_port_receive(flow)
+                         : odp_port_to_ofport(backer, flow->in_port.odp_port));
 
     flow->in_port.ofp_port = xport ? xport->ofp_port : OFPP_NONE;
     if (!xport) {
@@ -794,10 +794,10 @@ bucket_is_alive(const struct xlate_ctx *ctx,
     }
 
     return !ofputil_bucket_has_liveness(bucket) ||
-           (bucket->watch_port != OFPP_ANY &&
-            odp_port_is_alive(ctx, bucket->watch_port)) ||
-           (bucket->watch_group != OFPG_ANY &&
-            group_is_alive(ctx, bucket->watch_group, depth + 1));
+        (bucket->watch_port != OFPP_ANY &&
+         odp_port_is_alive(ctx, bucket->watch_port)) ||
+        (bucket->watch_group != OFPG_ANY &&
+         group_is_alive(ctx, bucket->watch_group, depth + 1));
 }
 
 static const struct ofputil_bucket *
@@ -1148,6 +1148,11 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
             /* No slaves enabled, so drop packet. */
             return;
         }
+
+        if (ctx->xin->resubmit_stats) {
+            bond_account(out_xbundle->bond, &ctx->xin->flow, vid,
+                         ctx->xin->resubmit_stats->n_bytes);
+        }
     }
 
     old_tci = *flow_tci;
@@ -1207,7 +1212,7 @@ is_mac_learning_update_needed(const struct mac_learning *ml,
                               const struct flow *flow,
                               struct flow_wildcards *wc,
                               int vlan, struct xbundle *in_xbundle)
-    OVS_REQ_RDLOCK(ml->rwlock)
+OVS_REQ_RDLOCK(ml->rwlock)
 {
     struct mac_entry *mac;
 
@@ -1247,7 +1252,7 @@ static void
 update_learning_table__(const struct xbridge *xbridge,
                         const struct flow *flow, struct flow_wildcards *wc,
                         int vlan, struct xbundle *in_xbundle)
-    OVS_REQ_WRLOCK(xbridge->ml->rwlock)
+OVS_REQ_WRLOCK(xbridge->ml->rwlock)
 {
     struct mac_entry *mac;
 
@@ -1356,7 +1361,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport *in_port,
                  || mac_entry_is_grat_arp_locked(mac))) {
                 ovs_rwlock_unlock(&xbridge->ml->rwlock);
                 xlate_report(ctx, "SLB bond thinks this packet looped back, "
-                            "dropping");
+                             "dropping");
                 return false;
             }
             ovs_rwlock_unlock(&xbridge->ml->rwlock);
@@ -3128,6 +3133,13 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
     }
 
     in_port = get_ofp_port(ctx.xbridge, flow->in_port.ofp_port);
+    if (in_port && in_port->is_tunnel && ctx.xin->resubmit_stats) {
+        netdev_vport_inc_rx(in_port->netdev, ctx.xin->resubmit_stats);
+        if (in_port->bfd) {
+            bfd_account_rx(in_port->bfd, ctx.xin->resubmit_stats);
+        }
+    }
+
     special = process_special(&ctx, flow, in_port, ctx.xin->packet);
     if (special) {
         ctx.xout->slow |= special;
@@ -3181,6 +3193,31 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
         ctx.xout->slow |= SLOW_ACTION;
     }
 
+    if (ctx.xin->resubmit_stats) {
+        mirror_update_stats(ctx.xbridge->mbridge, xout->mirrors,
+                            ctx.xin->resubmit_stats->n_packets,
+                            ctx.xin->resubmit_stats->n_bytes);
+
+        if (ctx.xbridge->netflow) {
+            const struct ofpact *ofpacts;
+            size_t ofpacts_len;
+
+            ofpacts_len = actions->ofpacts_len;
+            ofpacts = actions->ofpacts;
+            if (ofpacts_len == 0
+                || ofpacts->type != OFPACT_CONTROLLER
+                || ofpact_next(ofpacts) < ofpact_end(ofpacts, ofpacts_len)) {
+                /* Only update netflow if we don't have controller flow.  We don't
+                 * report NetFlow expiration messages for such facets because they
+                 * are just part of the control logic for the network, not real
+                 * traffic. */
+                netflow_flow_update(ctx.xbridge->netflow, flow,
+                                    xout->nf_output_iface,
+                                    ctx.xin->resubmit_stats);
+            }
+        }
+    }
+
     ofpbuf_uninit(&ctx.stack);
     ofpbuf_uninit(&ctx.action_set);
 
index 27bfa74..f4ef1e1 100644 (file)
@@ -155,8 +155,8 @@ int xlate_receive(const struct dpif_backer *, struct ofpbuf *packet,
 void xlate_actions(struct xlate_in *, struct xlate_out *)
     OVS_EXCLUDED(xlate_rwlock);
 void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
-                   const struct flow *, struct rule_dpif *,
-                   uint16_t tcp_flags, const struct ofpbuf *packet);
+                   const struct flow *, struct rule_dpif *, uint16_t tcp_flags,
+                   const struct ofpbuf *packet);
 void xlate_out_uninit(struct xlate_out *);
 void xlate_actions_for_side_effects(struct xlate_in *);
 void xlate_out_copy(struct xlate_out *dst, const struct xlate_out *src);
index ee08fb5..17ddb42 100644 (file)
@@ -286,7 +286,6 @@ struct facet {
     long long int prev_used;     /* Used time from last stats push. */
 
     /* Accounting. */
-    uint64_t accounted_bytes;    /* Bytes processed by facet_account(). */
     uint16_t tcp_flags;          /* TCP flags seen for this 'rule'. */
 
     struct xlate_out xout;
@@ -318,7 +317,6 @@ static void flow_push_stats(struct ofproto_dpif *, struct flow *,
                             struct dpif_flow_stats *, bool may_learn);
 static void facet_push_stats(struct facet *, bool may_learn);
 static void facet_learn(struct facet *);
-static void facet_account(struct facet *);
 static void push_all_stats(void);
 
 static bool facet_is_controller_flow(struct facet *);
@@ -2820,13 +2818,6 @@ get_ofp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
     return ofport ? ofport_dpif_cast(ofport) : NULL;
 }
 
-static struct ofport_dpif *
-get_odp_port(const struct ofproto_dpif *ofproto, odp_port_t odp_port)
-{
-    struct ofport_dpif *port = odp_port_to_ofport(ofproto->backer, odp_port);
-    return port && &ofproto->up == port->up.ofproto ? port : NULL;
-}
-
 static void
 ofproto_port_from_dpif_port(struct ofproto_dpif *ofproto,
                             struct ofproto_port *ofproto_port,
@@ -3582,10 +3573,8 @@ update_subfacet_stats(struct subfacet *subfacet,
     subfacet->dp_byte_count = stats->n_bytes;
     subfacet_update_stats(subfacet, &diff);
 
-    if (facet->accounted_bytes < facet->byte_count) {
+    if (diff.n_packets) {
         facet_learn(facet);
-        facet_account(facet);
-        facet->accounted_bytes = facet->byte_count;
     }
 }
 
@@ -3856,11 +3845,6 @@ facet_create(const struct flow_miss *miss)
     classifier_insert(&ofproto->facets, &facet->cr);
     ovs_rwlock_unlock(&ofproto->facets.rwlock);
 
-    if (ofproto->netflow && !facet_is_controller_flow(facet)) {
-        netflow_flow_update(ofproto->netflow, &facet->flow,
-                            facet->xout.nf_output_iface, &miss->stats);
-    }
-
     return facet;
 }
 
@@ -3980,54 +3964,6 @@ facet_learn(struct facet *facet)
     facet_push_stats(facet, true);
 }
 
-static void
-facet_account(struct facet *facet)
-{
-    const struct nlattr *a;
-    unsigned int left;
-    ovs_be16 vlan_tci;
-    uint64_t n_bytes;
-
-    if (!facet->xout.has_normal || !facet->ofproto->has_bonded_bundles) {
-        return;
-    }
-    n_bytes = facet->byte_count - facet->accounted_bytes;
-
-    /* This loop feeds byte counters to bond_account() for rebalancing to use
-     * as a basis.  We also need to track the actual VLAN on which the packet
-     * is going to be sent to ensure that it matches the one passed to
-     * bond_choose_output_slave().  (Otherwise, we will account to the wrong
-     * hash bucket.)
-     *
-     * We use the actions from an arbitrary subfacet because they should all
-     * be equally valid for our purpose. */
-    vlan_tci = facet->flow.vlan_tci;
-    NL_ATTR_FOR_EACH_UNSAFE (a, left, facet->xout.odp_actions.data,
-                             facet->xout.odp_actions.size) {
-        const struct ovs_action_push_vlan *vlan;
-        struct ofport_dpif *port;
-
-        switch (nl_attr_type(a)) {
-        case OVS_ACTION_ATTR_OUTPUT:
-            port = get_odp_port(facet->ofproto, nl_attr_get_odp_port(a));
-            if (port && port->bundle && port->bundle->bond) {
-                bond_account(port->bundle->bond, &facet->flow,
-                             vlan_tci_to_vid(vlan_tci), n_bytes);
-            }
-            break;
-
-        case OVS_ACTION_ATTR_POP_VLAN:
-            vlan_tci = htons(0);
-            break;
-
-        case OVS_ACTION_ATTR_PUSH_VLAN:
-            vlan = nl_attr_get(a);
-            vlan_tci = vlan->vlan_tci;
-            break;
-        }
-    }
-}
-
 /* Returns true if the only action for 'facet' is to send to the controller.
  * (We don't report NetFlow expiration messages for such facets because they
  * are just part of the control logic for the network, not real traffic). */
@@ -4074,10 +4010,6 @@ facet_flush_stats(struct facet *facet)
     }
 
     facet_push_stats(facet, false);
-    if (facet->accounted_bytes < facet->byte_count) {
-        facet_account(facet);
-        facet->accounted_bytes = facet->byte_count;
-    }
 
     if (ofproto->netflow && !facet_is_controller_flow(facet)) {
         netflow_expire(ofproto->netflow, &facet->flow);
@@ -4283,24 +4215,14 @@ facet_reset_counters(struct facet *facet)
     facet->byte_count = 0;
     facet->prev_packet_count = 0;
     facet->prev_byte_count = 0;
-    facet->accounted_bytes = 0;
 }
 
 static void
 flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow,
                 struct dpif_flow_stats *stats, bool may_learn)
 {
-    struct ofport_dpif *in_port;
     struct xlate_in xin;
 
-    in_port = get_ofp_port(ofproto, flow->in_port.ofp_port);
-    if (in_port && in_port->is_tunnel) {
-        netdev_vport_inc_rx(in_port->up.netdev, stats);
-        if (in_port->bfd) {
-            bfd_account_rx(in_port->bfd, stats);
-        }
-    }
-
     xlate_in_init(&xin, ofproto, flow, NULL, stats->tcp_flags, NULL);
     xin.resubmit_stats = stats;
     xin.may_learn = may_learn;
@@ -4325,13 +4247,6 @@ facet_push_stats(struct facet *facet, bool may_learn)
         facet->prev_packet_count = facet->packet_count;
         facet->prev_byte_count = facet->byte_count;
         facet->prev_used = facet->used;
-
-        if (facet->ofproto->netflow && !facet_is_controller_flow(facet)) {
-            netflow_flow_update(facet->ofproto->netflow, &facet->flow,
-                                facet->xout.nf_output_iface, &stats);
-        }
-        mirror_update_stats(facet->ofproto->mbridge, facet->xout.mirrors,
-                            stats.n_packets, stats.n_bytes);
         flow_push_stats(facet->ofproto, &facet->flow, &stats, may_learn);
     }
 }