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 <tony.vanderp...@alliedtelesis.co.nz>
Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.")
Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
---
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 bc5098a..36088b3 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_forward_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 53b7226..9e2450e 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_forward_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_forward_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_forward_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 ec7bd60..4f7d16d 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"])
--
2.1.4
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
