[ovs-dev] [PATCH v6] ofproto: per-table statistics

Simon Horman horms at verge.net.au
Thu May 22 05:44:36 UTC 2014


Add per-table counters. This resolves some short-comings
in the data provided in a table stats reply message.

* Lookups and matches are calculated based on table
  accesses rather than datapath flow hits and misses.

* Lookups and matches are credited to the table where they
  occurred rather than all being credited to table 0.

These problems were observed when running make check-ryu
and this patch allows many of its tester.py match checks
to pass.

Reviewed-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
Signed-off-by: Simon Horman <horms at verge.net.au>

--
v6
* Rebase

v5
* As suggested by Ben Pfaff
  - Use atomic_ulong instead of atomic_uint64_t as the later may
    not be portable.

v4
* Rebase

v3
* Add: Reviewed-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
* Rebase

v2
* Use NULL instead of 0 as NULL pointer argument
* Rebase
---
 ofproto/ofproto-dpif-xlate.c |   8 +-
 ofproto/ofproto-dpif.c       |  39 ++++-----
 ofproto/ofproto-dpif.h       |   5 +-
 ofproto/ofproto-provider.h   |   3 +
 ofproto/ofproto.c            |   3 +
 tests/ofproto-dpif.at        | 195 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 229 insertions(+), 24 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 93998fb..e36330d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2065,7 +2065,8 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
                                               ? &ctx->xout->wc : NULL,
                                               honor_table_miss,
                                               &ctx->table_id, &rule,
-                                              ctx->xin->xcache != NULL);
+                                              ctx->xin->xcache != NULL,
+                                              ctx->xin->resubmit_stats);
         ctx->xin->flow.in_port.ofp_port = old_in_port;
 
         if (ctx->xin->resubmit_hook) {
@@ -2693,7 +2694,7 @@ xlate_learn_action(struct xlate_ctx *ctx,
         /* Lookup the learned rule, taking a reference on it.  The reference
          * is released when this cache entry is deleted. */
         rule_dpif_lookup(ctx->xbridge->ofproto, &ctx->xin->flow, NULL,
-                         &entry->u.learn.rule, true);
+                         &entry->u.learn.rule, true, NULL);
     }
 }
 
@@ -3304,7 +3305,8 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
     if (!xin->ofpacts && !ctx.rule) {
         ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
                                         !xin->skip_wildcards ? wc : NULL,
-                                        &rule, ctx.xin->xcache != NULL);
+                                        &rule, ctx.xin->xcache != NULL,
+                                        ctx.xin->resubmit_stats);
         if (ctx.xin->resubmit_stats) {
             rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
         }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ab56728..9fb7975 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1526,25 +1526,20 @@ get_features(struct ofproto *ofproto_ OVS_UNUSED,
 }
 
 static void
-get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots)
+get_tables(struct ofproto *ofproto, struct ofp12_table_stats *ots)
 {
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-    struct dpif_dp_stats s;
-    uint64_t n_miss, n_no_pkt_in, n_bytes, n_dropped_frags;
-    uint64_t n_lookup;
-    long long int used;
+    int i;
 
     strcpy(ots->name, "classifier");
 
-    dpif_get_dp_stats(ofproto->backer->dpif, &s);
-    rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes, &used);
-    rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes,
-                   &used);
-    rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes,
-                   &used);
-    n_lookup = s.n_hit + s.n_missed - n_dropped_frags;
-    ots->lookup_count = htonll(n_lookup);
-    ots->matched_count = htonll(n_lookup - n_miss - n_no_pkt_in);
+    for (i = 0; i < ofproto->n_tables; i++) {
+        unsigned long missed, matched;
+
+        atomic_read(&ofproto->tables[i].n_matched, &matched);
+        ots[i].matched_count = htonll(matched);
+        atomic_read(&ofproto->tables[i].n_missed, &missed);
+        ots[i].lookup_count = htonll(matched + missed);
+    }
 }
 
 static struct ofport *
@@ -3203,7 +3198,7 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
 uint8_t
 rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
                  struct flow_wildcards *wc, struct rule_dpif **rule,
-                 bool take_ref)
+                 bool take_ref, const struct dpif_flow_stats *stats)
 {
     enum rule_dpif_lookup_verdict verdict;
     enum ofputil_port_config config = 0;
@@ -3230,7 +3225,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
     }
 
     verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
-                                          &table_id, rule, take_ref);
+                                          &table_id, rule, take_ref, stats);
 
     switch (verdict) {
     case RULE_DPIF_LOOKUP_VERDICT_MATCH:
@@ -3352,7 +3347,7 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
                             struct flow_wildcards *wc,
                             bool honor_table_miss,
                             uint8_t *table_id, struct rule_dpif **rule,
-                            bool take_ref)
+                            bool take_ref, const struct dpif_flow_stats *stats)
 {
     uint8_t next_id;
 
@@ -3363,6 +3358,12 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
         *table_id = next_id;
         *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc,
                                           take_ref);
+        if (stats) {
+            struct oftable *tbl = &ofproto->up.tables[next_id];
+            atomic_ulong *stat = *rule ? &tbl->n_matched : &tbl->n_missed;
+            unsigned long orig;
+            atomic_add(stat, stats->n_packets, &orig);
+        }
         if (*rule) {
             return RULE_DPIF_LOOKUP_VERDICT_MATCH;
         } else if (!honor_table_miss) {
@@ -4223,7 +4224,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
     if (ofpacts) {
         rule = NULL;
     } else {
-        rule_dpif_lookup(ofproto, flow, &trace.wc, &rule, false);
+        rule_dpif_lookup(ofproto, flow, &trace.wc, &rule, false, NULL);
 
         trace_format_rule(ds, 0, rule);
         if (rule == ofproto->miss_rule) {
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 06c4854..c238b24 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -89,7 +89,7 @@ bool ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *);
 
 uint8_t rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
                          struct flow_wildcards *, struct rule_dpif **rule,
-                         bool take_ref);
+                         bool take_ref, const struct dpif_flow_stats *);
 
 enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct ofproto_dpif *,
                                                           const struct flow *,
@@ -97,7 +97,8 @@ enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct ofproto_dpif *,
                                                           bool force_controller_on_miss,
                                                           uint8_t *table_id,
                                                           struct rule_dpif **rule, 
-                                                          bool take_ref);
+                                                          bool take_ref,
+                                                          const struct dpif_flow_stats *);
 
 static inline void rule_dpif_ref(struct rule_dpif *);
 static inline void rule_dpif_unref(struct rule_dpif *);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index e5f71c7..903105d 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -260,6 +260,9 @@ struct oftable {
 
     /* Table config: contains enum ofproto_table_config; accessed atomically. */
     atomic_uint config;
+
+    atomic_ulong n_matched;
+    atomic_ulong n_missed;
 };
 
 /* Assigns TABLE to each oftable, in turn, in OFPROTO.
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7fd4ac1..844f07b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6801,6 +6801,9 @@ oftable_init(struct oftable *table)
     classifier_set_prefix_fields(&table->cls, default_prefix_fields,
                                  ARRAY_SIZE(default_prefix_fields));
     fat_rwlock_unlock(&table->cls.rwlock);
+
+    atomic_init(&table->n_matched, 0);
+    atomic_init(&table->n_missed, 0);
 }
 
 /* Destroys 'table', including its classifier and eviction groups.
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b87f735..26532c1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4629,3 +4629,198 @@ AT_CHECK([grep -c 'resubmits yielded over 64 kB of stack' ovs-vswitchd.log], [0]
 ])
 OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"])
 AT_CLEANUP
+
+
+AT_SETUP([ofproto-if packet-out controller])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_CHECK([ovs-ofctl add-flow br0 'dl_dst=50:54:00:00:00:0a actions=controller'])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+	AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 CONTROLLER controller '50540000000a5054000000091234'])
+done
+
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+])
+
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ dl_dst=50:54:00:00:00:0a actions=CONTROLLER:65535
+NXST_FLOW reply:
+])
+
+(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): 254 tables"
+ echo "  0: active=1, lookup=0, matched=0"
+ x=1
+ while test $x -lt 254; do
+   echo "  $x: active=0, lookup=0, matched=0"
+   x=`expr $x + 1`
+ done) > expout
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
+AT_SETUP([ofproto-if packet-out controller (patch port)])
+OVS_VSWITCHD_START(
+  [-- \
+   add-port br0 p1 -- \
+   set interface p1 type=patch options:peer=p2 -- \
+   add-br br1 -- \
+   set bridge br1 datapath-type=dummy -- \
+   set bridge br1 fail-mode=secure -- \
+   set bridge br1 protocols='[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13]' -- \
+   add-port br1 p2 -- \
+   set interface p2 type=patch options:peer=p1 --])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br1 65534 invalid_ttl --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+	AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 CONTROLLER output:1 '50540000000a5054000000091234'])
+done
+
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via no_match) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via no_match) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via no_match) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+])
+
+(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): 254 tables"
+ x=0
+ while test $x -lt 254; do
+   echo "  $x: active=0, lookup=0, matched=0"
+   x=`expr $x + 1`
+ done) > expout
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout])
+
+(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): 254 tables"
+ echo "  0: active=0, lookup=3, matched=0"
+ x=1
+ while test $x -lt 254; do
+   echo "  $x: active=0, lookup=0, matched=0"
+   x=`expr $x + 1`
+ done) > expout
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br1 ], [0], [expout])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
+AT_SETUP([ofproto-if packet-out goto_table])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_DATA([flows.txt], [dnl
+table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1)
+table=1 dl_dst=50:54:00:00:00:0a actions=controller
+])
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
+done
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+])
+
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ n_packets=3, n_bytes=180, dl_dst=50:54:00:00:00:0a actions=goto_table:1
+ table=1, n_packets=3, n_bytes=180, dl_dst=50:54:00:00:00:0a actions=CONTROLLER:65535
+OFPST_FLOW reply (OF1.3):
+])
+
+(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): 254 tables"
+ echo "  0: active=1, lookup=3, matched=3"
+ echo "  1: active=1, lookup=3, matched=3"
+ x=2
+ while test $x -lt 254; do
+   echo "  $x: active=0, lookup=0, matched=0"
+   x=`expr $x + 1`
+ done) > expout
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
+AT_SETUP([ofproto-if packet-out table-miss (continue)])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'table=1 dl_dst=50:54:00:00:00:0a actions=controller'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 all continue])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
+done
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
+])
+
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ table=1, n_packets=3, n_bytes=180, dl_dst=50:54:00:00:00:0a actions=CONTROLLER:65535
+OFPST_FLOW reply (OF1.3):
+])
+
+(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): 254 tables"
+ echo "  0: active=0, lookup=3, matched=0"
+ echo "  1: active=1, lookup=3, matched=3"
+ x=2
+ while test $x -lt 254; do
+   echo "  $x: active=0, lookup=0, matched=0"
+   x=`expr $x + 1`
+ done) > expout
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
1.8.4




More information about the dev mailing list