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. Regards, Dumitru > }> } > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
