> 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
>>  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 <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

Reply via email to