Hi Ben, my 2c worth is that I agree that the stats are, at this time, inherently inaccurate and that this patch is an improvement over the current situation.
On Thu, Apr 03, 2014 at 08:46:38AM -0700, Ben Pfaff wrote: > We have had inaccuracies in OpenFlow flow stats for similar reasons, for > years, and it doesn't seem to bother people. I would like perfectly > accurate stats, but no one yet has come up with an inexpensive way to do > that. Given that, and because the existing stats are basically broken, > I am inclined to accept Simon's approach. What do you think? > > On Thu, Apr 03, 2014 at 08:01:45PM +0900, YAMAMOTO Takashi wrote: > > consider: > > > > 1. a datapath flow had hits > > 2. the userland flow removed > > 3. revalidation of the datapath flow failed > > > > with your patch, doesn't #3 count hits in #1 as misses? > > > > YAMAMOTO Takashi > > > > > 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. > > > > > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > > --- > > > ofproto/ofproto-dpif-xlate.c | 6 +- > > > ofproto/ofproto-dpif.c | 42 +++++----- > > > ofproto/ofproto-dpif.h | 5 +- > > > ofproto/ofproto-provider.h | 3 + > > > ofproto/ofproto.c | 2 + > > > tests/ofproto-dpif.at | 195 > > > +++++++++++++++++++++++++++++++++++++++++++ > > > 6 files changed, 231 insertions(+), 22 deletions(-) > > > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > > index a6bfc87..49f138a 100644 > > > --- a/ofproto/ofproto-dpif-xlate.c > > > +++ b/ofproto/ofproto-dpif-xlate.c > > > @@ -1901,7 +1901,9 @@ xlate_table_action(struct xlate_ctx *ctx, > > > ofp_port_t in_port, uint8_t table_id, > > > !skip_wildcards > > > ? &ctx->xout->wc : NULL, > > > honor_table_miss, > > > - &ctx->table_id, &rule); > > > + &ctx->table_id, > > > + ctx->xin->resubmit_stats, > > > + &rule); > > > ctx->xin->flow.in_port.ofp_port = old_in_port; > > > > > > if (ctx->xin->resubmit_hook) { > > > @@ -3057,7 +3059,7 @@ 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->resubmit_stats, &rule); > > > 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 ee97707..f4d7399 100644 > > > --- a/ofproto/ofproto-dpif.c > > > +++ b/ofproto/ofproto-dpif.c > > > @@ -1407,25 +1407,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++) { > > > + uint64_t 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 * > > > @@ -3082,14 +3077,15 @@ rule_dpif_get_actions(const struct rule_dpif > > > *rule) > > > * where misses occur. */ > > > uint8_t > > > rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow, > > > - struct flow_wildcards *wc, struct rule_dpif **rule) > > > + struct flow_wildcards *wc, const struct dpif_flow_stats > > > *stats, > > > + struct rule_dpif **rule) > > > { > > > enum rule_dpif_lookup_verdict verdict; > > > enum ofputil_port_config config = 0; > > > uint8_t table_id = 0; > > > > > > verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true, > > > - &table_id, rule); > > > + &table_id, stats, rule); > > > > > > switch (verdict) { > > > case RULE_DPIF_LOOKUP_VERDICT_MATCH: > > > @@ -3188,7 +3184,9 @@ rule_dpif_lookup_from_table(struct ofproto_dpif > > > *ofproto, > > > const struct flow *flow, > > > struct flow_wildcards *wc, > > > bool honor_table_miss, > > > - uint8_t *table_id, struct rule_dpif **rule) > > > + uint8_t *table_id, > > > + const struct dpif_flow_stats *stats, > > > + struct rule_dpif **rule) > > > { > > > uint8_t next_id; > > > > > > @@ -3198,6 +3196,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); > > > + if (stats) { > > > + struct oftable *tbl = &ofproto->up.tables[next_id]; > > > + atomic_uint64_t *stat = *rule ? &tbl->n_matched : > > > &tbl->n_missed; > > > + uint64_t orig; > > > + atomic_add(stat, stats->n_packets, &orig); > > > + } > > > if (*rule) { > > > return RULE_DPIF_LOOKUP_VERDICT_MATCH; > > > } else if (!honor_table_miss) { > > > @@ -4051,7 +4055,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const > > > struct flow *flow, > > > if (ofpacts) { > > > rule = NULL; > > > } else { > > > - rule_dpif_lookup(ofproto, flow, &trace.wc, &rule); > > > + rule_dpif_lookup(ofproto, flow, &trace.wc, 0, &rule); > > > > > > trace_format_rule(ds, 0, rule); > > > if (rule == ofproto->miss_rule) { > > > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > > > index 088ff89..3fa8f29 100644 > > > --- a/ofproto/ofproto-dpif.h > > > +++ b/ofproto/ofproto-dpif.h > > > @@ -78,13 +78,16 @@ extern struct ovs_rwlock xlate_rwlock; > > > size_t ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *); > > > > > > uint8_t rule_dpif_lookup(struct ofproto_dpif *, const struct flow *, > > > - struct flow_wildcards *, struct rule_dpif > > > **rule); > > > + struct flow_wildcards *, > > > + const struct dpif_flow_stats *, > > > + struct rule_dpif **rule); > > > > > > enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct > > > ofproto_dpif *, > > > const struct > > > flow *, > > > struct > > > flow_wildcards *, > > > bool > > > force_controller_on_miss, > > > uint8_t > > > *table_id, > > > + const struct > > > dpif_flow_stats *, > > > struct > > > rule_dpif **rule); > > > > > > void rule_dpif_ref(struct rule_dpif *); > > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > > > index a7bf4df..317be49 100644 > > > --- a/ofproto/ofproto-provider.h > > > +++ b/ofproto/ofproto-provider.h > > > @@ -264,6 +264,9 @@ struct oftable { > > > > > > /* Table config: contains enum ofp_table_config; accessed > > > atomically. */ > > > atomic_uint config; > > > + > > > + atomic_uint64_t n_matched; > > > + atomic_uint64_t n_missed; > > > }; > > > > > > /* Assigns TABLE to each oftable, in turn, in OFPROTO. > > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > > index 80336de..ac3a093 100644 > > > --- a/ofproto/ofproto.c > > > +++ b/ofproto/ofproto.c > > > @@ -6679,6 +6679,8 @@ oftable_init(struct oftable *table) > > > classifier_init(&table->cls, flow_segment_u32s); > > > table->max_flows = UINT_MAX; > > > atomic_init(&table->config, (unsigned > > > int)OFPTC11_TABLE_MISS_CONTROLLER); > > > + 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 d2234e9..890bcb4 100644 > > > --- a/tests/ofproto-dpif.at > > > +++ b/tests/ofproto-dpif.at > > > @@ -4330,3 +4330,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.5.2 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev