From: Ethan Jackson <ethan@nicira.com>
Date: Mon, 2 May 2011 20:15:59 +0000 (-0700)
Subject: ofproto: Resubmit statistics improperly account during failover.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6f1435fc8f77e925fbdb5e5dad91d61645330c6c;p=openvswitch

ofproto: Resubmit statistics improperly account during failover.

In some cases, when a facet's actions change because it resubmits
into a different rule, it will account all packets it as accrued
in the datapath to the new rule.  Due to the algorithm we are
using, it is acceptable for a facet to miscount at most 1 second
worth of packets in this manner.  This patch implements the proper
behavior.

Generally speaking, when a facet is facet_put__() into the
datapath, the kernel returns the old flow's statistics so they may
be accounted for in user space.  These statistics are generally
pushed down to the relevant facet's resubmit children.  Before this
patch, facet_put__() did not compensate for the fact that many of
the statistics in the datapath may have been already pushed.
Thus the entire packet count stored in the datapath would be pushed
to its children instead of simply the packets which have accrued
since the last accounting.  This patch fixes the behavior by
subtracting already accounted for packets from the statistics
returned by the datapath.
---

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 445f0d2b..4ed29143 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -268,6 +268,7 @@ static void facet_flush_stats(struct ofproto *, struct facet *);
 
 static void facet_make_actions(struct ofproto *, struct facet *,
                                const struct ofpbuf *packet);
+static void facet_reset_dp_stats(struct facet *, struct dpif_flow_stats *);
 static void facet_update_stats(struct ofproto *, struct facet *,
                                const struct dpif_flow_stats *);
 static void facet_push_stats(struct ofproto *, struct facet *);
@@ -1632,6 +1633,12 @@ facet_make_actions(struct ofproto *p, struct facet *facet,
     ofpbuf_delete(odp_actions);
 }
 
+/* Updates 'facet''s flow in the datapath setting its actions to 'actions_len'
+ * bytes of actions in 'actions'.  If 'stats' is non-null, statistics counters
+ * in the datapath will be zeroed and 'stats' will be updated with traffic new
+ * since 'facet' was last updated.
+ *
+ * Returns 0 if successful, otherwise a positive errno value.*/
 static int
 facet_put__(struct ofproto *ofproto, struct facet *facet,
             const struct nlattr *actions, size_t actions_len,
@@ -1640,19 +1647,24 @@ facet_put__(struct ofproto *ofproto, struct facet *facet,
     struct odputil_keybuf keybuf;
     enum dpif_flow_put_flags flags;
     struct ofpbuf key;
+    int ret;
 
     flags = DPIF_FP_CREATE | DPIF_FP_MODIFY;
     if (stats) {
         flags |= DPIF_FP_ZERO_STATS;
-        facet->dp_packet_count = 0;
-        facet->dp_byte_count = 0;
     }
 
     ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
     odp_flow_key_from_flow(&key, &facet->flow);
 
-    return dpif_flow_put(ofproto->dpif, flags, key.data, key.size,
-                         actions, actions_len, stats);
+    ret = dpif_flow_put(ofproto->dpif, flags, key.data, key.size,
+                        actions, actions_len, stats);
+
+    if (stats) {
+        facet_reset_dp_stats(facet, stats);
+    }
+
+    return ret;
 }
 
 /* If 'facet' is installable, inserts or re-inserts it into 'p''s datapath.  If
@@ -1696,16 +1708,18 @@ facet_uninstall(struct ofproto *p, struct facet *facet)
         struct odputil_keybuf keybuf;
         struct dpif_flow_stats stats;
         struct ofpbuf key;
+        int error;
 
         ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
         odp_flow_key_from_flow(&key, &facet->flow);
 
-        if (!dpif_flow_del(p->dpif, key.data, key.size, &stats)) {
+        error = dpif_flow_del(p->dpif, key.data, key.size, &stats);
+        facet_reset_dp_stats(facet, &stats);
+        if (!error) {
             facet_update_stats(p, facet, &stats);
         }
+
         facet->installed = false;
-        facet->dp_packet_count = 0;
-        facet->dp_byte_count = 0;
     } else {
         assert(facet->dp_packet_count == 0);
         assert(facet->dp_byte_count == 0);
@@ -1724,6 +1738,24 @@ facet_is_controller_flow(struct facet *facet)
                                       htons(OFPP_CONTROLLER)));
 }
 
+/* Resets 'facet''s datapath statistics counters.  This should be called when
+ * 'facet''s statistics are cleared in the datapath.  If 'stats' is non-null,
+ * it should contain the statistics returned by dpif when 'facet' was reset in
+ * the datapath.  'stats' will be modified to only included statistics new
+ * since 'facet' was last updated. */
+static void
+facet_reset_dp_stats(struct facet *facet, struct dpif_flow_stats *stats)
+{
+    if (stats && facet->dp_packet_count < stats->n_packets
+        && facet->dp_byte_count < stats->n_bytes) {
+        stats->n_packets -= facet->dp_packet_count;
+        stats->n_bytes -= facet->dp_byte_count;
+    }
+
+    facet->dp_packet_count = 0;
+    facet->dp_byte_count = 0;
+}
+
 /* Folds all of 'facet''s statistics into its rule.  Also updates the
  * accounting ofhook and emits a NetFlow expiration if appropriate.  All of
  * 'facet''s statistics in the datapath should have been zeroed and folded into