On Thu, Oct 8, 2020 at 7:47 AM Dumitru Ceara <[email protected]> wrote: > > On 10/2/20 8:25 PM, Han Zhou wrote: > > In a scenario in I-P when a desired flow is removed and an exactly same flow is > > added back in the same iteration, the merge_tracked_flows() function will merge > > the operation without firing any OpenFlow action. However, if there are > > multiple desired flows (e.g. F1 and F2) matching the same installed flow, and > > if the one being re-added (say, F1) is the one currently installed in OVS, the > > current implementation will update the currently installed flow to F2 in the > > data structure while the actual OVS flow is not updated (still F1). So far > > there is still no problem, but the member desired_flow of the installed flow > > is pointing to the wrong desired flow. > > > > Now there is only one place where the desired_flow member is consulted, in > > update_installed_flows_by_track() for handling a tracked *modified* flow. In > > reality flow modification happens only when conjunction flows need to be > > combined, which would never happen together with the above scenario of > > merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any > > real problem yet. > > > > However, there is a place that can already utilize this active desired_flow > > information, which is when handling a tracked flow deletion in > > update_installed_flows_by_track(): we can check if the flow being deleted is > > the currently active one installed in OVS. If not, we don't need to send and > > OpenFlow modify to OVS. This optimization is added in this patch, apart from > > fixing the problem of merge_tracked_flows(). Without the fix, this optimization > > would cause the test case added in this patch fail. > > > > Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") > > Signed-off-by: Han Zhou <[email protected]> > > --- > > controller/ofctrl.c | 28 +++++++++++++- > > tests/ovn.at | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 134 insertions(+), 2 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 81a00c8..e725c00 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) > > } > > } > > > > +/* Returns true if a desired flow is active (the one currently installed > > + * among the list of desired flows that are linked to the same installed > > + * flow). */ > > +static inline bool > > +desired_flow_is_active(struct desired_flow *d) > > +{ > > + return (d->installed_flow && d->installed_flow->desired_flow == d); > > +} > > + > > +/* Set a desired flow as the active one among the list of desired flows > > + * that are linked to the same installed flow. */ > > +static inline void > > +desired_flow_set_active(struct desired_flow *d) > > +{ > > + ovs_assert(d->installed_flow); > > + d->installed_flow->desired_flow = d; > > +} > > + > > static void > > link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) > > { > > @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > /* del_f must have been installed, otherwise it should have been > > * removed during track_flow_add_or_modify. */ > > ovs_assert(del_f->installed_flow); > > + > > + bool del_f_was_active = desired_flow_is_active(del_f); > > if (!f->installed_flow) { > > /* f is not installed yet. */ > > struct installed_flow *i = del_f->installed_flow; > > @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > ovs_assert(f->installed_flow == del_f->installed_flow); > > unlink_installed_to_desired(del_f->installed_flow, del_f); > > } > > + if (del_f_was_active) { > > + desired_flow_set_active(f); > > Nit: I think in this case the separate API makes it a bit more > complicated to read the code. We already set > "installed_flow->desired_flow" directly in 3 other places. I think it's > fine to just: > > f->installed_flow->desired_flow = f; > > Except for this the rest looks good to me: > > Acked-by: Dumitru Ceara <[email protected]> > > Thanks, > Dumitru > Thanks Dumitru for the ack! I applied this to master. As agreed with Dumitru in a quick chat on IRC, I kept the patch as is, because "is_active()" made code more readable and a corresponding "set_active()" was necessary to make the interface consistent.
Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
