[ovs-dev] [PATCH] ofproto: Properly refresh rule modified time when nothing else changes.

Justin Pettit jpettit at nicira.com
Sat Jan 26 01:43:27 UTC 2013


I'm going to backport it to branch-1.9.  I had to make the following change for the unit test:

-=-=-=-=-=-=-=-=-=-
diff --git a/tests/learn.at b/tests/learn.at
index 305d2f8..868a4b5 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -120,9 +120,9 @@ dnl it gets re-learned again the next time a packet appears,
 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])
+  [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
 
@@ -138,7 +138,7 @@ flow="in_port(3),eth(src=50:54:00:00:00:07,dst=50:54:00:00:0
 AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
-expected="1,2,100"
+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])
-=-=-=-=-=-=-=-=-=-

Does that look reasonable to you?  If so, I'll push it.

--Justin


On Jan 25, 2013, at 5:24 PM, Ben Pfaff <blp at nicira.com> wrote:

> Thanks, I applied this to master.  I know we talked about applying it
> to earlier branches too, but I didn't do that yet.
> 
> On Fri, Jan 25, 2013 at 04:51:51PM -0800, Justin Pettit wrote:
>> Looks good.
>> 
>> --Justin
>> 
>> 
>> On Jan 25, 2013, at 3:00 PM, Ben Pfaff <blp at nicira.com> wrote:
>> 
>>> 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
>>> 
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>> 




More information about the dev mailing list