From 8b6987d799fb0bc530ebb7f767767b1c661548c9 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Thu, 1 Dec 2016 13:10:53 -0800 Subject: [PATCH] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message. While a flow modify must keep the original flow's flags, it must reset counts if (and only if) the reset_counts flag is present in the flow mod message. Behavior prior to this patch is broken in a few ways: - OpenFlow 1.0 and 1.1 mod-flows did reset the counts, if the flow had reset_counts flag set. Only add-flow should reset counts. - With OpenFlow 1.2 and later, if the old flow had the reset_counts flag set, the counts would be reset by mod-flows, even if the flow-mod message does not have the reset_counts flag set. - With OpenFlow 1.2 and later, mod-flows with a reset_count did not reset the counts, if the old flow did not have the reset_counts flag set. Even though the prevailing interpretation seems to be that the reset_counts flag in the flow-mod message should be stored as part of the flow state (and reported back in flow dumps with OpenFlow >= 1.3), we should always just look at the reset_counts flag in the current flow-mod and ignore the reset_counts flag stored in the flow when processing a flow mod. For OpenFlow 1.0 and 1.1 we already implicitly add the reset_counts flag for add-flow messages (only) to maintain the expected behavior. This patch adds a comprehensive test case to prevent future regressions. Suggested-by: Tony van der Peet Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.") Signed-off-by: Jarno Rajahalme Acked-by: Ben Pfaff --- ofproto/ofproto-provider.h | 1 + ofproto/ofproto.c | 10 ++- tests/ofproto-dpif.at | 174 +++++++++++++++++++++++++++++++++++++ 3 files changed, 181 insertions(+), 4 deletions(-) diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index bc5098a9e61..c515779d7a8 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1870,6 +1870,7 @@ struct ofproto_flow_mod { bool modify_cookie; /* Fields derived from ofputil_flow_mod. */ bool modify_may_add_flow; + bool modify_keep_counts; enum nx_flow_update_event event; /* These are only used during commit execution. diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 53b722665e0..4f43c45e28e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -5160,7 +5160,6 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, struct ovs_list *dead_cookies) OVS_REQUIRES(ofproto_mutex) { - bool forward_counts = !(new_rule->flags & OFPUTIL_FF_RESET_COUNTS); struct rule *replaced_rule; replaced_rule = (old_rule && old_rule->removed_reason != OFPRR_EVICTION) @@ -5169,10 +5168,10 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, /* Insert the new flow to the ofproto provider. A non-NULL 'replaced_rule' * is a duplicate rule the 'new_rule' is replacing. The provider should * link the packet and byte counts from the old rule to the new one if - * 'forward_counts' is 'true'. The 'replaced_rule' will be deleted right - * after this call. */ + * 'modify_keep_counts' is 'true'. The 'replaced_rule' will be deleted + * right after this call. */ ofproto->ofproto_class->rule_insert(new_rule, replaced_rule, - forward_counts); + ofm->modify_keep_counts); learned_cookies_inc(ofproto, rule_get_actions(new_rule)); if (old_rule) { @@ -7331,6 +7330,9 @@ ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, ofm->modify_may_add_flow = (fm->new_cookie != OVS_BE64_MAX && fm->cookie_mask == htonll(0)); + /* Old flags must be kept when modifying a flow, but we still must + * honor the reset counts flag if present in the flow mod. */ + ofm->modify_keep_counts = !(fm->flags & OFPUTIL_FF_RESET_COUNTS); /* Initialize state needed by ofproto_flow_mod_uninit(). */ ofm->temp_rule = rule; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index ec7bd60d43e..4f7d16d8502 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6460,6 +6460,180 @@ AT_CHECK([strip_xids < stdout | sed -n 's/duration=[[0-9]]*\.[[0-9]]*s/duration= OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - flow stats reset_counts]) +OVS_VSWITCHD_START +flow="ip,actions=NORMAL" + +ovs-appctl time/stop + +AT_CHECK([ovs-ofctl add-flow br0 $flow]) + +warp_and_dump_NXM () { + AT_CHECK([ovs-appctl time/warp 1000], [0], [ignore]) + AT_CHECK([ovs-appctl revalidator/purge], [0]) + + AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br0], [0], [stdout]) + if [[ $5 -gt 0 ]]; then + expected=" cookie=0x0, duration=$1s, table=0, n_packets=$2, n_bytes=$3, idle_age=$4, hard_age=$5, ip actions=NORMAL" + else + expected=" cookie=0x0, duration=$1s, table=0, n_packets=$2, n_bytes=$3, idle_age=$4, ip actions=NORMAL" + fi + AT_CHECK_UNQUOTED([strip_xids < stdout | sed -n 's/duration=\([[0-9]]*\)\.*[[0-9]]*s/duration=\1s/p' | sort], [0], [dnl +$expected +]) +} + +warp_and_dump_OF () { + AT_CHECK([ovs-appctl time/warp 1000], [0], [ignore]) + AT_CHECK([ovs-appctl revalidator/purge], [0]) + + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 dump-flows br0], [0], [stdout]) + if [[ $1 -lt 13 -o "$5X" = "X" ]]; then + expected=" cookie=0x0, duration=$2s, table=0, n_packets=$3, n_bytes=$4, ip actions=NORMAL" + else + expected=" cookie=0x0, duration=$2s, table=0, n_packets=$3, n_bytes=$4, $5 ip actions=NORMAL" + fi + AT_CHECK_UNQUOTED([strip_xids < stdout | sed -n 's/duration=\([[0-9]]*\)\.*[[0-9]]*s/duration=\1s/p' | sort], [0], [dnl +$expected +]) +} + +send_packet () { + ovs-appctl netdev-dummy/receive br0 'in_port(0),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no)' +} + +# OpenFlow 1.0, implicit reset_counts +send_packet +warp_and_dump_NXM 1 1 54 1 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow10 add-flow br0 $flow]) +# add-flow resets duration and counts, +# but idle age is inherited from the old flow +warp_and_dump_NXM 1 0 0 2 + +send_packet +warp_and_dump_NXM 2 1 54 1 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow10 mod-flows br0 $flow]) +# mod-flows resets hard_age, but not counts +# but duration and idle_age is inherited from the old flow +warp_and_dump_NXM 3 1 54 2 1 + +# OpenFlow 1.1, implicit reset_counts +send_packet +warp_and_dump_OF 11 4 2 108 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow11 add-flow br0 $flow]) +# add-flow resets duration and counts, +# but idle age is inherited from the old flow +warp_and_dump_NXM 1 0 0 2 +warp_and_dump_OF 11 2 0 0 + +send_packet +warp_and_dump_OF 11 3 1 54 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow11 mod-flows br0 $flow]) +# mod-flows resets hard_age, but not counts +# but duration and idle_age is inherited from the old flow +warp_and_dump_NXM 4 1 54 2 1 +warp_and_dump_OF 11 5 1 54 + +# OpenFlow 1.2, explicit reset_counts +send_packet +warp_and_dump_OF 12 6 2 108 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 add-flow br0 $flow]) +# add-flow without flags resets duration, but not counts, +# idle age is inherited from the old flow +warp_and_dump_NXM 1 2 108 2 +warp_and_dump_OF 12 2 2 108 + +send_packet +warp_and_dump_OF 12 3 3 162 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 mod-flows br0 $flow]) +# mod-flows without flags does not reset duration nor counts, +# idle age is inherited from the old flow +warp_and_dump_NXM 4 3 162 2 1 +warp_and_dump_OF 12 5 3 162 + +send_packet +warp_and_dump_OF 12 6 4 216 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 add-flow br0 reset_counts,$flow]) +# add-flow with reset_counts resets both duration and counts, +# idle age is inherited from the old flow +warp_and_dump_NXM 1 0 0 2 +warp_and_dump_OF 12 2 0 0 + +send_packet +warp_and_dump_OF 12 3 1 54 +AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 mod-flows br0 reset_counts,$flow]) +# mod-flows with reset_counts resets counts, but not duration, +# idle age is inherited from the old flow +warp_and_dump_NXM 4 0 0 2 1 +warp_and_dump_OF 12 5 0 0 + +# OpenFlow > 1.3, explicit reset_counts +flow_mods_reset_counts () { + # Reset to a known state + AT_CHECK([ovs-ofctl add-flow br0 $flow]) + + send_packet + warp_and_dump_OF $1 1 1 54 reset_counts + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 $flow]) + # add-flow without flags resets duration, but not counts, + # idle age is inherited from the old flow + warp_and_dump_NXM 1 1 54 2 + warp_and_dump_OF $1 2 1 54 + + send_packet + warp_and_dump_OF $1 3 2 108 + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 $flow]) + # mod-flows without flags does not reset duration nor counts, + # idle age is inherited from the old flow + warp_and_dump_NXM 4 2 108 2 1 + warp_and_dump_OF $1 5 2 108 + + send_packet + warp_and_dump_OF $1 6 3 162 + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 reset_counts,$flow]) + # add-flow with reset_counts resets both duration and counts, + # idle age is inherited from the old flow + warp_and_dump_NXM 1 0 0 2 + warp_and_dump_OF $1 2 0 0 reset_counts + + send_packet + warp_and_dump_OF $1 3 1 54 reset_counts + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 reset_counts,$flow]) + # mod-flows with reset_counts resets counts, but not duration, + # idle age is inherited from the old flow + warp_and_dump_NXM 4 0 0 2 1 + warp_and_dump_OF $1 5 0 0 reset_counts + + # Modify flow having reset_counts flag without reset_counts + send_packet + warp_and_dump_OF $1 6 1 54 reset_counts + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 $flow]) + warp_and_dump_NXM 7 1 54 2 1 + warp_and_dump_OF $1 8 1 54 reset_counts + + # Add flow having reset_counts flag without reset_counts + send_packet + warp_and_dump_OF $1 9 2 108 reset_counts + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 $flow]) + warp_and_dump_NXM 1 2 108 2 + warp_and_dump_OF $1 2 2 108 + + # Modify flow w/o reset_counts flag with a flow_mod having reset_counts + send_packet + warp_and_dump_OF $1 3 3 162 + AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 reset_counts,$flow]) + warp_and_dump_NXM 4 0 0 2 1 + warp_and_dump_OF $1 5 0 0 +} + +# OpenFlow versions >= 1.3 should behave the same way +flow_mods_reset_counts 13 +flow_mods_reset_counts 14 +flow_mods_reset_counts 15 + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - flow stats, set-n-threads]) OVS_VSWITCHD_START AT_CHECK([ovs-ofctl add-flow br0 "ip,actions=NORMAL"])