> On Nov 29, 2016, at 3:46 PM, Tony van der Peet 
> <tony.vanderp...@alliedtelesis.co.nz> wrote:
> 
> If the reset_counts flag is set on a flow modification message, the
> flow counters must be cleared, even if the flow does not already have
> the reset_counts flag set. And the flow modification message is not
> able to set the reset_counts flag in the flow, so the flag must be
> tracked separately and not saved in the flow state.

Thanks for pointing this out!

Looking into this I noticed that the behavior was even more broken, most likely 
due to refactoring done by me earlier:

- OpenFlow 1.0 and 1.1 mod-flows did reset the counts, while only add-flow 
should
- 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 mod-flows message does not 
have the reset_counts flag set.
- And as you noted, 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.

So, 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. For OpenFlow 1.0 and 1.1 we implicitly add the 
reset_counts flag for add-flow messages (only) to maintain the expected 
behavior.

> ---
> include/openvswitch/ofp-util.h | 2 ++
> ofproto/ofproto.c              | 5 ++++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
> index 8703d2a..b7a32dc 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -277,6 +277,8 @@ enum ofputil_flow_mod_flags {
>                                           set or modified. */
>     OFPUTIL_FF_NO_READONLY   = 1 << 7, /* Allow rules within read only tables
>                                           to be modified */
> +    OFPUTIL_FF_MOD_RESET_COU = 1 << 8, /* Reset counters for a flow 
> modification
> +                                          or delete message */
> };
> 

Instead of adding more confusion to the stored flow state, it is better to 
store the reset_counts flag in the (decoded) flow-mod message to the run-time 
context in ofproto.

I posted a patch yesterday with the fix and a test case verifying the behavior. 
I’d be happy if you could test and review it!

  Jarno

> /* Protocol-independent flow_mod.
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 53b7226..5af1828 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -5094,6 +5094,9 @@ replace_rule_start(struct ofproto *ofproto, struct 
> ofproto_flow_mod *ofm,
>                 new_rule->idle_timeout = old_rule->idle_timeout;
>                 new_rule->hard_timeout = old_rule->hard_timeout;
>                 *CONST_CAST(uint16_t *, &new_rule->importance) = 
> old_rule->importance;
> +                if (new_rule->flags & OFPUTIL_FF_RESET_COUNTS) {
> +                    old_rule->flags |= OFPUTIL_FF_MOD_RESET_COU;
> +                }
>                 new_rule->flags = old_rule->flags;
>                 new_rule->created = old_rule->created;
>             }
> @@ -5160,7 +5163,7 @@ replace_rule_finish(struct ofproto *ofproto, struct 
> ofproto_flow_mod *ofm,
>                     struct ovs_list *dead_cookies)
>     OVS_REQUIRES(ofproto_mutex)
> {
> -    bool forward_counts = !(new_rule->flags & OFPUTIL_FF_RESET_COUNTS);
> +    bool forward_counts = !(new_rule->flags & (OFPUTIL_FF_RESET_COUNTS | 
> OFPUTIL_FF_MOD_RESET_COU));
>     struct rule *replaced_rule;
> 
>     replaced_rule = (old_rule && old_rule->removed_reason != OFPRR_EVICTION)
> -- 
> 2.10.2
> 
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Reply via email to