[ovs-dev] [PATCH ovn] ofctrl.c: Avoid repeatedly linking an installed flow and a desired flow.

Han Zhou hzhou at ovn.org
Thu Oct 8 19:43:06 UTC 2020


On Thu, Oct 8, 2020 at 8:47 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 10/2/20 10:05 PM, Han Zhou wrote:
> > In update_installed_flows_by_compare() there are two loops. The first
> > loop iterates the installed flows and find its peer in desired flows to
> > 1. uninstall flows that are not needed anymore
> > 2. update flows if needed
> > At the same time, it links the desired flow found for the installed flow
> > which also set the desired flow as the current active installed flow.
> >
> > The second loop iterates the desired flows and find its peer in
installed
> > flows to install missing flows. At the same time it will detect if there
> > are conflict desired flows matching same installed flow then just link
> > them.
> >
> > However, currently in the second loop, it blindly link the desired flows
> > to the installed flows, without checking if it is already linked. Lucky
> > enough, this won't cause any real problem so far, because when there are
> > conflict flows, the one found in the first loop will also be the one
> > traversed first in the second loop. After the first loop, each installed
> > flow will be linked to one and only one desired flow, and in the second
> > loop the same ones will be readded to the list - readding the only
element
> > in the list becomes a no-op.
> >
> > However, this is still a bug and this patch fixes it to avoid potential
> > problems and confusion in the code.
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
> >  controller/ofctrl.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 81a00c844..3276b45b0 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -1687,8 +1687,12 @@ update_installed_flows_by_compare(struct
ovn_desired_flow_table *flow_table,
> >              /* Copy 'd' from 'flow_table' to installed_flows. */
> >              i = installed_flow_dup(d);
> >              hmap_insert(&installed_flows, &i->match_hmap_node,
i->flow.hash);
> > +            link_installed_to_desired(i, d);
> > +        } else if (i->desired_flow != d) {
> > +            /* This is a desired_flow that conflicts with one installed
> > +             * previously. */
> > +            link_installed_to_desired(i, d);
> >          }
> > -        link_installed_to_desired(i, d);
>
> Hi Han,
>
> Maybe I'm missing something but link_installed_to_desired(i, d) already
> returns early if i->desired_flow == d.  It seems to me that the
> unpatched version of the code was doing the same thing as it does with
> your patch applied.
>

Thanks Dumitru. You are right. It was still confusing. I reworked it with
v2. Please take a look:
https://patchwork.ozlabs.org/project/ovn/patch/20201008193402.3786783-1-hzhou@ovn.org/


More information about the dev mailing list