ofproto: Update 'struct facet''s comments.

[openvswitch] / ofproto / ofproto-dpif.c

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a6e1782e492e203b24dad61384279c3acd4dfd7e..3fd95ea6e162668c9be01eadfda45a25b06c129e 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -207,12 +207,8 @@ struct facet {
      *     dpif_execute().
      *
      *   - Do include packets and bytes that were obtained from the datapath
-     *     when a flow was deleted (e.g. dpif_flow_del()) or when its
-     *     statistics were reset (e.g. dpif_flow_put() with
+     *     when its statistics were reset (e.g. dpif_flow_put() with
      *     DPIF_FP_ZERO_STATS).
-     *
-     *   - Do not include any packets or bytes that can currently be obtained
-     *     from the datapath by, e.g., dpif_flow_get().
      */
     uint64_t packet_count;       /* Number of packets received. */
     uint64_t byte_count;         /* Number of bytes received. */
@@ -224,10 +220,7 @@ struct facet {
     uint64_t rs_byte_count;      /* Bytes pushed to resubmit children. */
     long long int rs_used;       /* Used time pushed to resubmit children. */
 
-    /* Number of bytes passed to account_cb.  This may include bytes that can
-     * currently obtained from the datapath (thus, it can be greater than
-     * byte_count). */
-    uint64_t accounted_bytes;
+    uint64_t accounted_bytes;    /* Bytes processed by facet_account(). */
 
     struct hmap_node hmap_node;  /* In owning ofproto's 'facets' hmap. */
     struct list list_node;       /* In owning rule's 'facets' list. */
@@ -269,10 +262,10 @@ static void facet_update_time(struct ofproto_dpif *, struct facet *,
                               long long int used);
 static void facet_update_stats(struct ofproto_dpif *, struct facet *,
                                const struct dpif_flow_stats *);
+static void facet_reset_counters(struct facet *);
 static void facet_reset_dp_stats(struct facet *, struct dpif_flow_stats *);
 static void facet_push_stats(struct facet *);
-static void facet_account(struct ofproto_dpif *, struct facet *,
-                          uint64_t extra_bytes);
+static void facet_account(struct ofproto_dpif *, struct facet *);
 
 static bool facet_is_controller_flow(struct facet *);
 
@@ -1848,7 +1841,7 @@ update_stats(struct ofproto_dpif *p)
             facet->dp_byte_count = stats->n_bytes;
 
             facet_update_time(p, facet, stats->used);
-            facet_account(p, facet, stats->n_bytes);
+            facet_account(p, facet);
             facet_push_stats(facet);
         } else {
             /* There's a flow in the datapath that we know nothing about.
@@ -2223,10 +2216,9 @@ vlan_tci_to_openflow_vlan(ovs_be16 vlan_tci)
 }
 
 static void
-facet_account(struct ofproto_dpif *ofproto,
-              struct facet *facet, uint64_t extra_bytes)
+facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
 {
-    uint64_t total_bytes, n_bytes;
+    uint64_t n_bytes;
     struct ofbundle *in_bundle;
     const struct nlattr *a;
     tag_type dummy = 0;
@@ -2234,12 +2226,11 @@ facet_account(struct ofproto_dpif *ofproto,
     ovs_be16 vlan_tci;
     int vlan;
 
-    total_bytes = facet->byte_count + extra_bytes;
-    if (total_bytes <= facet->accounted_bytes) {
+    if (facet->byte_count <= facet->accounted_bytes) {
         return;
     }
-    n_bytes = total_bytes - facet->accounted_bytes;
-    facet->accounted_bytes = total_bytes;
+    n_bytes = facet->byte_count - facet->accounted_bytes;
+    facet->accounted_bytes = facet->byte_count;
 
     /* Test that 'tags' is nonzero to ensure that only flows that include an
      * OFPP_NORMAL action are used for learning and bond slave rebalancing.
@@ -2355,7 +2346,7 @@ facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
     assert(!facet->dp_packet_count);
 
     facet_push_stats(facet);
-    facet_account(ofproto, facet, 0);
+    facet_account(ofproto, facet);
 
     if (ofproto->netflow && !facet_is_controller_flow(facet)) {
         struct ofexpired expired;
@@ -2371,11 +2362,7 @@ facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
 
     /* Reset counters to prevent double counting if 'facet' ever gets
      * reinstalled. */
-    facet->packet_count = 0;
-    facet->byte_count = 0;
-    facet->rs_packet_count = 0;
-    facet->rs_byte_count = 0;
-    facet->accounted_bytes = 0;
+    facet_reset_counters(facet);
 
     netflow_flow_clear(&facet->nf_flow);
 }
@@ -2537,6 +2524,16 @@ facet_update_stats(struct ofproto_dpif *ofproto, struct facet *facet,
     }
 }
 
+static void
+facet_reset_counters(struct facet *facet)
+{
+    facet->packet_count = 0;
+    facet->byte_count = 0;
+    facet->rs_packet_count = 0;
+    facet->rs_byte_count = 0;
+    facet->accounted_bytes = 0;
+}
+
 static void
 facet_push_stats(struct facet *facet)
 {
@@ -2663,6 +2660,13 @@ rule_construct(struct rule *rule_)
         rule->facets = victim->facets;
         list_moved(&rule->facets);
         LIST_FOR_EACH (facet, list_node, &rule->facets) {
+            /* XXX: We're only clearing our local counters here.  It's possible
+             * that quite a few packets are unaccounted for in the datapath
+             * statistics.  These will be accounted to the new rule instead of
+             * cleared as required.  This could be fixed by clearing out the
+             * datapath statistics for this facet, but currently it doesn't
+             * seem worth it. */
+            facet_reset_counters(facet);
             facet->rule = rule;
         }
     } else {