| Message ID | 20201002200504.2954064-1-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofctrl.c: Avoid repeatedly linking an installed flow and a desired flow. | expand |
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@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 > }> } > >
On Thu, Oct 8, 2020 at 8:47 AM Dumitru Ceara <dceara@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@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/
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); } }
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@ovn.org> --- controller/ofctrl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)