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

[Han Zhou]

On Thu, Oct 8, 2020 at 7:47 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> 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
>
Thanks Dumitru for the ack! I applied this to master.
As agreed with Dumitru in a quick chat on IRC, I kept the patch as is,
because "is_active()" made code more readable and a corresponding
"set_active()" was necessary to make the interface consistent.

Thanks,
Han