ofproto-dpif-xlate: Add xlate cache type XC_TABLE.

Xlate cache entry type XC_TABLE is required for the table stats (number of misses and matches) to be correctly attributed. It appears that table stats have been off ever since xlate cache was introduced. This was now revealed by a PACKET_OUT unit test case in a later patch that checks for table stats explicitly. Fixes: b256dc5 ("ofproto-dpif-xlate: Cache xlate_actions() effects.") Signed-off-by: Jarno Rajahalme <jarno@ovn.org> Acked-by: Ben Pfaff <blp@ovn.org>
openvswitch · Sep 14, 2016 · a027899 · a027899
1 parent 901a517
commit a027899
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 5 deletions.
diff --git a/NEWS b/NEWS
@@ -1,5 +1,8 @@
 Post-v2.6.0
 ---------------------
+   - Fixed regression in table stats maintenance introduced in OVS
+     2.3.0, wherein the number of OpenFlow table hits and misses was
+     not accurate.
    - OpenFlow:
      * Bundles now expire after 10 seconds since the last time the
        bundle was either opened, modified, or closed.

diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c
@@ -88,6 +88,14 @@ xlate_push_stats_entry(struct xc_entry *entry,
     struct eth_addr dmac;
 
     switch (entry->type) {
+    case XC_TABLE:
+        ofproto_dpif_credit_table_stats(entry->table.ofproto,
+                                        entry->table.id,
+                                        entry->table.match
+                                        ? stats->n_packets : 0,
+                                        entry->table.match
+                                        ? 0 : stats->n_packets);
+        break;
     case XC_RULE:
         rule_dpif_credit_stats(entry->rule, stats);
         break;
@@ -178,6 +186,8 @@ void
 xlate_cache_clear_entry(struct xc_entry *entry)
 {
     switch (entry->type) {
+    case XC_TABLE:
+        break;
     case XC_RULE:
         rule_dpif_unref(entry->rule);
         break;

diff --git a/ofproto/ofproto-dpif-xlate-cache.h b/ofproto/ofproto-dpif-xlate-cache.h
@@ -40,6 +40,7 @@ struct ofputil_flow_mod;
 struct rule_dpif;
 
 enum xc_type {
+    XC_TABLE,
     XC_RULE,
     XC_BOND,
     XC_NETDEV,
@@ -64,6 +65,11 @@ enum xc_type {
 struct xc_entry {
     enum xc_type type;
     union {
+        struct {
+            struct ofproto_dpif *ofproto;
+            uint8_t id;
+            bool    match; /* or miss. */
+        } table;
         struct rule_dpif *rule;
         struct {
             struct netdev *tx;

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
@@ -3188,8 +3188,8 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
                                            &ctx->xin->flow, ctx->wc,
                                            ctx->xin->resubmit_stats,
                                            &ctx->table_id, in_port,
-                                           may_packet_in, honor_table_miss);
-
+                                           may_packet_in, honor_table_miss,
+                                           ctx->xin->xcache);
         if (OVS_UNLIKELY(ctx->xin->resubmit_hook)) {
             ctx->xin->resubmit_hook(ctx->xin, rule, ctx->indentation + 1);
         }
@@ -5336,7 +5336,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         ctx.rule = rule_dpif_lookup_from_table(
             ctx.xbridge->ofproto, ctx.tables_version, flow, ctx.wc,
             ctx.xin->resubmit_stats, &ctx.table_id,
-            flow->in_port.ofp_port, true, true);
+            flow->in_port.ofp_port, true, true, ctx.xin->xcache);
         if (ctx.xin->resubmit_stats) {
             rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats);
         }

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
@@ -49,6 +49,7 @@
 #include "ofproto-dpif-sflow.h"
 #include "ofproto-dpif-upcall.h"
 #include "ofproto-dpif-xlate.h"
+#include "ofproto-dpif-xlate-cache.h"
 #include "openvswitch/ofp-actions.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/meta-flow.h"
@@ -3914,6 +3915,21 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, ovs_version_t version,
                                                                flow, wc)));
 }
 
+void
+ofproto_dpif_credit_table_stats(struct ofproto_dpif *ofproto, uint8_t table_id,
+                                uint64_t n_matches, uint64_t n_misses)
+{
+    struct oftable *tbl = &ofproto->up.tables[table_id];
+    unsigned long orig;
+
+    if (n_matches) {
+        atomic_add_relaxed(&tbl->n_matched, n_matches, &orig);
+    }
+    if (n_misses) {
+        atomic_add_relaxed(&tbl->n_missed, n_misses, &orig);
+    }
+}
+
 /* Look up 'flow' in 'ofproto''s classifier version 'version', starting from
  * table '*table_id'.  Returns the rule that was found, which may be one of the
  * special rules according to packet miss hadling.  If 'may_packet_in' is
@@ -3945,7 +3961,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
                             struct flow_wildcards *wc,
                             const struct dpif_flow_stats *stats,
                             uint8_t *table_id, ofp_port_t in_port,
-                            bool may_packet_in, bool honor_table_miss)
+                            bool may_packet_in, bool honor_table_miss,
+                            struct xlate_cache *xcache)
 {
     ovs_be16 old_tp_src = flow->tp_src, old_tp_dst = flow->tp_dst;
     ofp_port_t old_in_port = flow->in_port.ofp_port;
@@ -3971,6 +3988,14 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
 
                 atomic_add_relaxed(&tbl->n_matched, stats->n_packets, &orig);
             }
+            if (xcache) {
+                struct xc_entry *entry;
+
+                entry = xlate_cache_add_entry(xcache, XC_TABLE);
+                entry->table.ofproto = ofproto;
+                entry->table.id = *table_id;
+                entry->table.match = true;
+            }
             return rule;
         }
     }
@@ -3999,6 +4024,14 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
             atomic_add_relaxed(rule ? &tbl->n_matched : &tbl->n_missed,
                                stats->n_packets, &orig);
         }
+        if (xcache) {
+            struct xc_entry *entry;
+
+            entry = xlate_cache_add_entry(xcache, XC_TABLE);
+            entry->table.ofproto = ofproto;
+            entry->table.id = next_id;
+            entry->table.match = (rule != NULL);
+        }
         if (rule) {
             goto out;   /* Match. */
         }

diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
@@ -102,14 +102,20 @@ struct dpif_backer_support *ofproto_dpif_get_support(const struct ofproto_dpif *
 
 ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
 
+void ofproto_dpif_credit_table_stats(struct ofproto_dpif *, uint8_t table_id,
+                                     uint64_t n_matches, uint64_t n_misses);
+
+struct xlate_cache;
+
 struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *,
                                               ovs_version_t, struct flow *,
                                               struct flow_wildcards *,
                                               const struct dpif_flow_stats *,
                                               uint8_t *table_id,
                                               ofp_port_t in_port,
                                               bool may_packet_in,
-                                              bool honor_table_miss);
+                                              bool honor_table_miss,
+                                              struct xlate_cache *xcache);
 
 static inline void rule_dpif_ref(struct rule_dpif *);
 static inline void rule_dpif_unref(struct rule_dpif *);