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

[Dumitru Ceara]

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.

Regards,
Dumitru

>      }>  }
>  
>