[ovs-dev] [PATCH ovn] ofctrl: Don't assert while merging tracked flows.
Han Zhou
hzhou at ovn.org
Wed Apr 28 07:31:18 UTC 2021
Thanks Numan for working on this issue!
On Mon, Apr 26, 2021 at 3:53 AM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at ovn.org>
>
> Presently we do ovs_assert(del_f->installed_flows) in
> merged_tracked_flows() of ofctrl.c if 'del_f' and flow 'f' are equal
> and if 'del_f' is not installed. But there are a couple of scenarios
> this can happen:
>
> 1. A flow 'F1' was added to the desired flows, but ofctrl_put() couldn't
> install the flow (because of the rconn queue is full) and an SB update
> caused 'F1' to be removed and re-added as 'F2'.
>
This seems not possible, because del_f->installed_flow is set whenever a
"installed_flow" is added to the installed_flows table, regardless of
whether it is sent to OVS or not.
> 2. In a single engine run, a flow 'F1' was added to the desired flow,
> removed from the desired flow and re-added as 'F2'. This could
> happen after the commit [1].
In theory this should not happen either, because if F1 was added and then
removed before it was installed, it would have been removed in
track_flow_del():
if (!f->installed_flow) {
/* If it is not installed yet, simply destroy it. */
desired_flow_destroy(f);
return;
}
In fact this is what the comment is supposed to say, but the comment had a
typo (my bad):
/* del_f must have been installed, otherwise it should have been
* removed during track_flow_add_or_modify. */
s/track_flow_add_or_modify/track_flow_del
In practice, if commit [1] did trigger this bt, then I'd look deeper into
the logic, but I think removing the assert may just hide the problem.
>
> The likelihood of (2) seems to be higher with datapath groups enabled.
>
> The assertion - ovs_assert(del_f->installed_flows) is observed in
> few deployments after the commit [1]. This patch addressed this issue
> by removing that assertion. Removing this assertion should not have
> any side effects as the flow 'F2' will be installed.
>
> This patch is proposed based on the code inspection and the possibility
> that the above mentioned scenarios can happen. The patch doesn't have
> any test cases to reproduce these scenarios.
>
> [1] - c6c61b4e3462("ofctrl: Fix the assert seen when flood removing flows
with conj actions.")
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1951502
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> controller/ofctrl.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index c29c3d180..9e960b034 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1931,11 +1931,29 @@ merge_tracked_flows(struct ovn_desired_flow_table
*flow_table)
> continue;
> }
>
> - /* del_f must have been installed, otherwise it should have
been
> - * removed during track_flow_add_or_modify. */
> - ovs_assert(del_f->installed_flow);
> + if (!del_f->installed_flow) {
> + /* del_f must have been installed, otherwise it should
have
> + * been removed during track_flow_add_or_modify.
> + *
> + * But however there are a couple of scenarios this may
not
> + * happen.
> + *
> + * Scenario 1: A flow was added to the desired flows,
but
> + * ofctrl_put() couldn't install the flow
and
> + * an SB update caused the 'del_f' to be
removed
> + * and re-added as 'f'.
> + *
> + * Scenario 2: In a single engine run, a flow 'del_f'
was
> + * added to the desired flow, removed from
the
> + * desired flow and re-added as 'f'. This
could
> + * happen after the commit c6c61b4e3462.
> + *
> + * Treat both the scenarios as valid scenarios and just
remove
> + * 'del_f' from the hmap - deleted_flows.
> + * update_installed_flows_by_track() will install 'f'.
> + */
>
> - if (!f->installed_flow) {
> + } else if (!f->installed_flow) {
> /* f is not installed yet. */
> replace_installed_to_desired(del_f->installed_flow,
del_f, f);
> } else {
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list