On Thu, Oct 8, 2020 at 8:47 AM Dumitru Ceara <[email protected]> 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 <[email protected]> > > --- > > 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/[email protected]/ _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
