[ovs-dev] [PATCH ovn] ofctrl.c: Fix duplicated flow handling in I-P while merging opposite changes.

[Dumitru Ceara]

On 10/2/20 8:25 PM, Han Zhou wrote:
> In a scenario in I-P when a desired flow is removed and an exactly same flow is
> added back in the same iteration, the merge_tracked_flows() function will merge
> the operation without firing any OpenFlow action. However, if there are
> multiple desired flows (e.g. F1 and F2) matching the same installed flow, and
> if the one being re-added (say, F1) is the one currently installed in OVS, the
> current implementation will update the currently installed flow to F2 in the
> data structure while the actual OVS flow is not updated (still F1). So far
> there is still no problem, but the member desired_flow of the installed flow
> is pointing to the wrong desired flow.
> 
> Now there is only one place where the desired_flow member is consulted, in
> update_installed_flows_by_track() for handling a tracked *modified* flow. In
> reality flow modification happens only when conjunction flows need to be
> combined, which would never happen together with the above scenario of
> merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any
> real problem yet.
> 
> However, there is a place that can already utilize this active desired_flow
> information, which is when handling a tracked flow deletion in
> update_installed_flows_by_track(): we can check if the flow being deleted is
> the currently active one installed in OVS. If not, we don't need to send and
> OpenFlow modify to OVS. This optimization is added in this patch, apart from
> fixing the problem of merge_tracked_flows(). Without the fix, this optimization
> would cause the test case added in this patch fail.
> 
> Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.")
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
>  controller/ofctrl.c |  28 +++++++++++++-
>  tests/ovn.at        | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 134 insertions(+), 2 deletions(-)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 81a00c8..e725c00 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
>      }
>  }
>  

> +/* Returns true if a desired flow is active (the one currently installed
> + * among the list of desired flows that are linked to the same installed
> + * flow). */
> +static inline bool
> +desired_flow_is_active(struct desired_flow *d)
> +{
> +    return (d->installed_flow && d->installed_flow->desired_flow == d);
> +}
> +
> +/* Set a desired flow as the active one among the list of desired flows
> + * that are linked to the same installed flow. */
> +static inline void
> +desired_flow_set_active(struct desired_flow *d)
> +{
> +    ovs_assert(d->installed_flow);
> +    d->installed_flow->desired_flow = d;
> +}
> +
>  static void
>  link_installed_to_desired(struct installed_flow *i, struct desired_flow *d)
>  {
> @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table)
>              /* del_f must have been installed, otherwise it should have been
>               * removed during track_flow_add_or_modify. */
>              ovs_assert(del_f->installed_flow);
> +
> +            bool del_f_was_active = desired_flow_is_active(del_f);
>              if (!f->installed_flow) {
>                  /* f is not installed yet. */
>                  struct installed_flow *i = del_f->installed_flow;
> @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table)
>                  ovs_assert(f->installed_flow == del_f->installed_flow);
>                  unlink_installed_to_desired(del_f->installed_flow, del_f);
>              }
> +            if (del_f_was_active) {
> +                desired_flow_set_active(f);

Nit: I think in this case the separate API makes it a bit more
complicated to read the code.  We already set
"installed_flow->desired_flow" directly in 3 other places.  I think it's
fine to just:

f->installed_flow->desired_flow = f;

Except for this the rest looks good to me:

Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks,
Dumitru