> On Nov 30, 2016, at 9:06 PM, Ben Pfaff <[email protected]> wrote: > > On Wed, Nov 30, 2016 at 05:51:12PM -0800, Jarno Rajahalme wrote: >> 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: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.") >> Signed-off-by: Jarno Rajahalme <[email protected]> > > This is tagged "net-next", but it's an OVS patch. >
Oops, my bad :-) > This is very nice. One rarely sees such a thorough test case. It had > never occurred to me to try to match exact values for duration, > idle_age, and hard_age in output (at least down to a number of seconds), > but if it works that's very nice too. > Seems to work fine with time/stop + time/warp! > If I may be permitted to nit-pick, the name "modify_forward_counts" took > me a bit of thinking to properly understand. Maybe "modify_keep_stats" > would be easier for me to understand at first glance. > “stats” include the last used timestamp, which is treated independently of the byte and packet counts. How about “modify_keep_counts”? > Tony, does this solve the problem for you? > > Acked-by: Ben Pfaff <[email protected] <mailto:[email protected]>> Thanks for the review! Jarno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
