secchan: Fix flow statistics tracking.

Updates of flow statistics have been ad hoc and somewhat broken for some
time now.  This commit makes them much more systematic and more likely
to be correct.

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 6ea28c7f293f1fbfe89a95957d1a3d2edf2abbad..30e07fce4ae33c79bc5d39be39d892f3b622efb3 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -104,8 +104,8 @@ struct rule {
     uint16_t hard_timeout;      /* In seconds from time of creation. */
     long long int used;         /* Last-used time (0 if never used). */
     long long int created;      /* Creation time. */
-    uint64_t packet_count;      /* Packets from *expired* subrules. */
-    uint64_t byte_count;        /* Bytes from *expired* subrules. */
+    uint64_t packet_count;      /* Number of packets received. */
+    uint64_t byte_count;        /* Number of bytes received. */
     uint8_t tcp_flags;          /* Bitwise-OR of all TCP flags seen. */
     uint8_t ip_tos;             /* Last-seen IP type-of-service. */
     tag_type tags;              /* Tags (set only by hooks). */
@@ -169,8 +169,9 @@ static void rule_remove(struct ofproto *, struct rule *);
 static bool rule_make_actions(struct ofproto *, struct rule *,
                               const struct ofpbuf *packet);
 static void rule_install(struct ofproto *, struct rule *,
-                         struct odp_flow_stats *);
+                         struct rule *displaced_rule);
 static void rule_uninstall(struct ofproto *, struct rule *);
+static void rule_post_uninstall(struct ofproto *, struct rule *);
 
 struct ofconn {
     struct list node;
@@ -1416,14 +1417,8 @@ rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet)
         /* We might need to change the rules for arbitrary subrules. */
         p->need_revalidate = true;
     } else {
-        struct odp_flow_stats stats;
-
         rule_make_actions(p, rule, packet);
-        rule_install(p, rule, &stats);
-
-        if (displaced_rule && displaced_rule->super) {
-            update_stats(displaced_rule->super, &stats);
-        }
+        rule_install(p, rule, displaced_rule);
     }
     if (displaced_rule) {
         rule_destroy(p, displaced_rule);
@@ -1486,7 +1481,7 @@ rule_make_actions(struct ofproto *p, struct rule *rule,
 
 static void
 rule_install(struct ofproto *p, struct rule *rule,
-             struct odp_flow_stats *stats)
+             struct rule *displaced_rule)
 {
     assert(!rule->cr.wc.wildcards);
 
@@ -1498,10 +1493,13 @@ rule_install(struct ofproto *p, struct rule *rule,
         odp_flow.n_actions = rule->n_odp_actions;
         if (!dpif_flow_add(&p->dpif, &odp_flow)) {
             rule->installed = true;
-            if (stats) {
-                *stats = odp_flow.stats;
+            if (displaced_rule) {
+                update_stats(rule, &odp_flow.stats);
+                rule_post_uninstall(p, displaced_rule);
             }
         }
+    } else if (displaced_rule) {
+        rule_uninstall(p, displaced_rule);
     }
 }
 
@@ -1512,6 +1510,8 @@ rule_update_actions(struct ofproto *ofproto, struct rule *rule)
     if (rule->may_install) {
         if (rule->installed) {
             if (actions_changed) {
+                /* XXX should really do rule_post_uninstall() for the *old* set
+                 * of actions, and distinguish the old stats from the new. */
                 dpif_flow_set_actions(&ofproto->dpif, &rule->cr.flow,
                                       rule->odp_actions, rule->n_odp_actions);
             }
@@ -1529,26 +1529,50 @@ rule_uninstall(struct ofproto *p, struct rule *rule)
     assert(!rule->cr.wc.wildcards);
     if (rule->installed) {
         struct odp_flow odp_flow;
+
         odp_flow.key = rule->cr.flow;
         odp_flow.actions = NULL;
         odp_flow.n_actions = 0;
         if (!dpif_flow_del(&p->dpif, &odp_flow)) {
-            update_stats(rule->super ? rule->super : rule, &odp_flow.stats);
+            update_stats(rule, &odp_flow.stats);
         }
         rule->installed = false;
 
-        if (p->netflow) {
-            struct ofexpired expired;
-            expired.flow = rule->cr.flow;
-            expired.packet_count = rule->packet_count;
-            expired.byte_count = rule->byte_count;
-            expired.used = rule->used;
-            expired.created = rule->created;
-            expired.tcp_flags = rule->tcp_flags;
-            expired.ip_tos = rule->ip_tos;
-            netflow_expire(p->netflow, &expired);
+        rule_post_uninstall(p, rule);
+    }
+}
+
+static void
+rule_post_uninstall(struct ofproto *ofproto, struct rule *rule)
+{
+    struct rule *super = rule->super;
+
+    if (ofproto->netflow) {
+        struct ofexpired expired;
+        expired.flow = rule->cr.flow;
+        expired.packet_count = rule->packet_count;
+        expired.byte_count = rule->byte_count;
+        expired.used = rule->used;
+        expired.created = rule->created;
+        expired.tcp_flags = rule->tcp_flags;
+        expired.ip_tos = rule->ip_tos;
+        netflow_expire(ofproto->netflow, &expired);
+    }
+    if (super) {
+        super->packet_count += rule->packet_count;
+        super->byte_count += rule->byte_count;
+        super->tcp_flags |= rule->tcp_flags;
+        if (rule->packet_count) {
+            super->ip_tos = rule->ip_tos;
         }
     }
+
+    /* Reset counters to prevent double counting if the rule ever gets
+     * reinstalled. */
+    rule->packet_count = 0;
+    rule->byte_count = 0;
+    rule->tcp_flags = 0;
+    rule->ip_tos = 0;
 }
 \f
 static void
@@ -2433,7 +2457,9 @@ update_stats(struct rule *rule, const struct odp_flow_stats *stats)
     rule->packet_count += stats->n_packets;
     rule->byte_count += stats->n_bytes;
     rule->tcp_flags |= stats->tcp_flags;
-    rule->ip_tos = stats->ip_tos;
+    if (stats->n_packets) {
+        rule->ip_tos = stats->ip_tos;
+    }
 }
 
 static int