[ovs-dev] [PATCH] ofproto: Properly refresh rule modified time when nothing else changes.
Ben Pfaff
blp at nicira.com
Fri Jan 25 23:00:28 UTC 2013
In Open vSwitch, a "modify" or "modify_strict" flow_mod is supposed to
refresh the flow's last-modified time even if nothing else changes, because
this interpretation makes the "learn" action more useful. As commit
308881afb (ofproto: Reinterpret meaning of OpenFlow hard timeouts with
OFPFC_MODIFY.) notes:
I finally found a good use for hard timeouts in OpenFlow, but they
require a slight reinterpretation of the meaning of hard timeouts.
Until now, a hard timeout meant that a flow would be removed the
specified number of seconds after a flow was created. Intervening
modifications with OFPFC_MODIFY(_STRICT) had no effect on the hard
timeout; the flow would still be deleted the specified number of
seconds after its original creation.
This commit changes the effect of OFPFC_MODIFY(_STRICT). Now,
modifying a flow resets its hard timeout counter. A flow will time out
the specified number of seconds after creation or after the last time
it is modified, whichever comes later.
However, commit 080437614b (ofproto: Represent flow cookie changes as
operations too.) broke this behavior because it incorrectly optimized out
"modify" operations that didn't change the flow's actions or flow cookie.
This commit fixes the problem, and adds a test to prevent future
regression.
Thanks to Amar Padmanabhan <amar at nicira.com> for helping to track this
down.
Bug #14538.
Reported-by: Hiroshi Tanaka <htanaka at vmware.com>
CC: Amar Padmanabhan <amar at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
ofproto/ofproto.c | 18 +++++++++---
tests/learn.at | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bc27079..249f2d5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3261,10 +3261,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
new_cookie = (fm->new_cookie != htonll(UINT64_MAX)
? fm->new_cookie
: rule->flow_cookie);
- if (!actions_changed && new_cookie == rule->flow_cookie) {
- /* No change at all. */
- continue;
- }
op = ofoperation_create(group, rule, OFOPERATION_MODIFY, 0);
rule->flow_cookie = new_cookie;
@@ -4247,7 +4243,19 @@ ofopgroup_complete(struct ofopgroup *group)
LIST_FOR_EACH_SAFE (op, next_op, group_node, &group->ops) {
struct rule *rule = op->rule;
- if (!op->error && !ofproto_rule_is_hidden(rule)) {
+ /* We generally want to report the change to active OpenFlow flow
+ monitors (e.g. NXST_FLOW_MONITOR). There are three exceptions:
+
+ - The operation failed.
+
+ - The affected rule is not visible to controllers.
+
+ - The operation's only effect was to update rule->modified. */
+ if (!(op->error
+ || ofproto_rule_is_hidden(rule)
+ || (op->type == OFOPERATION_MODIFY
+ && op->ofpacts
+ && rule->flow_cookie == op->flow_cookie))) {
/* Check that we can just cast from ofoperation_type to
* nx_flow_update_event. */
BUILD_ASSERT_DECL((enum nx_flow_update_event) OFOPERATION_ADD
diff --git a/tests/learn.at b/tests/learn.at
index 47c1d32..800dc14 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -122,6 +122,81 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+dnl This test checks that repeated uses of a "learn" action cause the
+dnl modified time of the learned flow to advance. Otherwise, the
+dnl learned flow will expire after its hard timeout even though it's
+dnl supposed to be refreshed. (The expiration can be hard to see since
+dnl it gets re-learned again the next time a packet appears, but
+dnl sometimes the expiration can cause temporary flooding etc.)
+AT_SETUP([learning action - learn refreshes hard_age])
+OVS_VSWITCHD_START(
+ [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
+ add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2 -- \
+ add-port br0 p3 -- set Interface p3 type=dummy ofport_request=3])
+
+ovs-appctl time/stop
+
+# Set up flow table for MAC learning.
+AT_DATA([flows.txt], [[
+table=0 actions=learn(table=1, hard_timeout=10, NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], output:NXM_OF_IN_PORT[]), resubmit(,1)
+table=1 priority=0 actions=flood
+]])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+# Trace an ICMP packet arriving on port 3, to create a MAC learning entry.
+flow="in_port(3),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=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)"
+AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+
+expected="1,2,100"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
+
+# Check that the MAC learning entry appeared.
+AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], [dnl
+ table=1, hard_timeout=10, dl_dst=50:54:00:00:00:07 actions=output:3
+ table=1, priority=0 actions=FLOOD
+NXST_FLOW reply:
+])
+
+# For 25 seconds, make sure that the MAC learning entry doesn't
+# disappear as long as we refresh it every second.
+for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25; do
+ ovs-appctl time/warp 1000
+ AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+
+ # Check that the entry is there.
+ AT_CHECK([ovs-ofctl dump-flows br0 table=1], [0], [stdout])
+ AT_CHECK([ofctl_strip < stdout | sort], [0], [dnl
+ table=1, hard_timeout=10, dl_dst=50:54:00:00:00:07 actions=output:3
+ table=1, priority=0 actions=FLOOD
+NXST_FLOW reply:
+])
+
+ if test $i != 1; then
+ # Check that hard_age has appeared. We need to do this separately
+ # from the above check because ofctl_strip removes it. dump-flows
+ # only prints hard_age when it is different from the flow's duration
+ # (that is, the number of seconds from the time it was created),
+ # so we only check for it after we've refreshed the flow once.
+ AT_CHECK([grep dl_dst=50:54:00:00:00:07 stdout | grep -c hard_age],
+ [0], [1
+])
+ fi
+done
+
+# Make sure that 15 seconds without refreshing makes the flow time out.
+ovs-appctl time/warp 5000
+ovs-appctl time/warp 5000
+ovs-appctl time/warp 5000
+ AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], [dnl
+ table=1, priority=0 actions=FLOOD
+NXST_FLOW reply:
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([learning action - TCPv4 port learning])
OVS_VSWITCHD_START(
[add-port br0 p1 -- set Interface p1 type=dummy -- \
--
1.7.2.5
More information about the dev
mailing list