Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-05 Thread Jarno Rajahalme

> On Dec 1, 2016, at 3:44 PM, Ben Pfaff  wrote:
> 
> On Thu, Dec 01, 2016 at 01:09:12PM -0800, Jarno Rajahalme wrote:
>>> 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”?
> 
> Sure.


Pushed to master and branch-2.6.

Backported the earlier used-timestamp fix to branch-2.5, and squashed in a fix 
for that patch, since it was the one introducing the reset_counts regression, 
and also squashed in the new test case to verify behavior on branch-2.5. 
Branch-2.5 is OK apart from a difference in reported packet size (54 vs. 60 
bytes). Since that difference is unrelated, I modified the test case and pushed 
the result to branch-2.5.

  Jarno

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-01 Thread Tony van der Peet
Thanks Ben, you are right. I was reading the test specification incorrectly. In 
fact, the code is working perfectly (evidence in the log file), it's my test 
procedure that's at fault.

I'll shut up now after thanking both you and Jarno for the previous patch!

Tony

From: Ben Pfaff <b...@ovn.org>
Sent: Friday, 2 December 2016 12:51 p.m.
To: Tony van der Peet
Cc: Jarno Rajahalme; d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS 
flag in flow modify message.

On Thu, Dec 01, 2016 at 09:37:19PM +, Tony van der Peet wrote:
> I will foreshadow another problem I found while implementing test
> suite 140. This is where flow is created without SEND_FLOW_REM flag,
> but flow delete does set this bit. The flow removed message is not
> sent in this case. I can see a similar solution to the current patch
> working equally effectively. I'll review Jarno's test cases and try to
> come up with a similarly comprehensive set for this new case.

This is some kind of new interpretation of how flags work.
Historically, flags are set only when a flow is added.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-01 Thread Ben Pfaff
On Thu, Dec 01, 2016 at 09:37:19PM +, Tony van der Peet wrote:
> I will foreshadow another problem I found while implementing test
> suite 140. This is where flow is created without SEND_FLOW_REM flag,
> but flow delete does set this bit. The flow removed message is not
> sent in this case. I can see a similar solution to the current patch
> working equally effectively. I'll review Jarno's test cases and try to
> come up with a similarly comprehensive set for this new case.

This is some kind of new interpretation of how flags work.
Historically, flags are set only when a flow is added.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-01 Thread Ben Pfaff
On Thu, Dec 01, 2016 at 01:09:12PM -0800, Jarno Rajahalme wrote:
> > 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”?

Sure.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-01 Thread Tony van der Peet
Hi Ben, Jarno

> Tony, does this solve the problem for you?

Yes it does. I like the solution presented by Jarno a bit more than mine too, I 
had a feeling that adding a new flag would cause problems.

I will foreshadow another problem I found while implementing test suite 140. 
This is where flow is created without SEND_FLOW_REM flag, but flow delete does 
set this bit. The flow removed message is not sent in this case. I can see a 
similar solution to the current patch working equally effectively. I'll review 
Jarno's test cases and try to come up with a similarly comprehensive set for 
this new case.

Cheers
Tony
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-12-01 Thread Jarno Rajahalme

> On Nov 30, 2016, at 9:06 PM, Ben Pfaff  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 
>> Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.")
>> Signed-off-by: Jarno Rajahalme 
> 
> 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 >

Thanks for the review!

  Jarno

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v2] ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

2016-11-30 Thread Ben Pfaff
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 
> Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.")
> Signed-off-by: Jarno Rajahalme 

This is tagged "net-next", but it's an OVS patch.

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.

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.

Tony, does this solve the problem for you?

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev