> 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?
i'm not necessarily against his approach. just a question. i agree that it's an improvement over the current situation. YAMAMOTO Takashi > > 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev