Skip to content

Commit

Permalink
ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Fixes: 748eb2f ("ofproto-dpif: Always forward 'used' from the old_rule.")
Signed-off-by: Jarno Rajahalme <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
Jarno Rajahalme committed Dec 5, 2016
1 parent 742c0ac commit 8b6987d
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 4 deletions.
1 change: 1 addition & 0 deletions ofproto/ofproto-provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 6 additions & 4 deletions ofproto/ofproto.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
174 changes: 174 additions & 0 deletions tests/ofproto-dpif.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand Down

0 comments on commit 8b6987d

Please sign in to comment.