From: Ben Pfaff Date: Fri, 25 Jan 2013 23:07:36 +0000 (-0800) Subject: ofproto: Properly refresh rule modified time when nothing else changes. X-Git-Tag: v1.9.0~12 X-Git-Url: http://git.cascardo.eti.br/?p=cascardo%2Fovs.git;a=commitdiff_plain;h=fea1494c0ebaa949da0c962980dc369e82fb4b5e ofproto: Properly refresh rule modified time when nothing else changes. 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 for helping to track this down. Bug #14841. Reported-by: Hiroshi Tanaka CC: Amar Padmanabhan Signed-off-by: Ben Pfaff --- diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 5bc6ee94a..f7d025600 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3058,10 +3058,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; @@ -4022,7 +4018,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 da82f51f2..868a4b5b6 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -112,6 +112,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 -- \ + add-port br0 p2 -- set Interface p2 type=dummy -- \ + add-port br0 p3 -- set Interface p3 type=dummy]) + +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="0,1,2" +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 eth0 -- set Interface eth0 type=dummy -- \