> On Nov 30, 2016, at 9:06 PM, Ben Pfaff <b...@ovn.org> 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
>> 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 <tony.vanderp...@alliedtelesis.co.nz>
>> Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.")
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> 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 <b...@ovn.org <mailto:b...@ovn.org>>
Thanks for the review!
dev mailing list