> On Nov 29, 2016, at 3:46 PM, Tony van der Peet
> <[email protected]> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
