[ovs-dev] [PATCH ovn v2 9/9] ofctrl.c: Merge opposite changes of tracked flows before installing.

[Mark Michelson]

On 9/7/20 2:45 AM, Han Zhou wrote:
> This patch optimizes the previous patch that incrementally processes
> flow installation by merging the "add-after-delete" flow changes as
> much as possible to avoid unnecessary OpenFlow updates.
> 
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
>   controller/ofctrl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 74 insertions(+)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index a2aa45d..81a00c8 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1692,10 +1692,84 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table,
>       }
>   }
>   
> +/* Finds and returns a desired_flow in 'deleted_flows' that is exactly the
> + * same as 'target', including cookie and actions.
> + */
> +static struct desired_flow *
> +deleted_flow_lookup(struct hmap *deleted_flows, struct ovn_flow *target)
> +{
> +    struct desired_flow *d;
> +    HMAP_FOR_EACH_WITH_HASH (d, match_hmap_node, target->hash,
> +                             deleted_flows) {
> +        struct ovn_flow *f = &d->flow;
> +        if (f->table_id == target->table_id
> +            && f->priority == target->priority
> +            && minimatch_equal(&f->match, &target->match)
> +            && f->cookie == target->cookie
> +            && ofpacts_equal(f->ofpacts, f->ofpacts_len, target->ofpacts,
> +                             target->ofpacts_len)) {
> +            return d;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +/* This function scans the tracked flow changes in the order and merges "add"
> + * or "update" after "deleted" operations for exactly same flow (priority,
> + * table, match, action and cookie), to avoid unnecessary OF messages being
> + * sent to OVS. */
> +static void
> +merge_tracked_flows(struct ovn_desired_flow_table *flow_table)
> +{
> +    struct hmap deleted_flows = HMAP_INITIALIZER(&deleted_flows);
> +    struct desired_flow *f, *next;
> +    LIST_FOR_EACH_SAFE (f, next, track_list_node,
> +                        &flow_table->tracked_flows) {
> +        if (f->is_deleted) {
> +            /* reuse f->match_hmap_node field since it is already removed from
> +             * the desired flow table's match index. */
> +            hmap_insert(&deleted_flows, &f->match_hmap_node,
> +                        f->flow.hash);
> +        } else {
> +            struct desired_flow *del_f = deleted_flow_lookup(&deleted_flows,
> +                                                             &f->flow);
> +            if (!del_f) {
> +                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 (!f->installed_flow) {
> +                /* f is not installed yet. */
> +                struct installed_flow *i = del_f->installed_flow;
> +                unlink_installed_to_desired(i, del_f);
> +                link_installed_to_desired(i, f);
> +            } else {
> +                /* f has been installed before, and now was updated to exact
> +                 * the same flow as del_f. */
> +                ovs_assert(f->installed_flow == del_f->installed_flow);
> +                unlink_installed_to_desired(del_f->installed_flow, del_f);
> +            }
> +            hmap_remove(&deleted_flows, &del_f->match_hmap_node);
> +            ovs_list_remove(&del_f->track_list_node);
> +            desired_flow_destroy(del_f);
> +
> +            ovs_list_remove(&f->track_list_node);
> +            ovs_list_init(&f->track_list_node);
> +        }
> +    }

What happens here if the deleted flow comes later in the list than the 
added/modified flows? Can that happen? If so, then this needs to be 
separated into two separate list traversals. The first adds all 
f->is_deleted flows to deleted_flows, and the second modifies the links 
on all !f->is_deleted flows that are found in deleted_flows.

> +    HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &deleted_flows) {
> +        hmap_remove(&deleted_flows, &f->match_hmap_node);
> +    }
> +    hmap_destroy(&deleted_flows);
> +}
> +
>   static void
>   update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,
>                                   struct ovs_list *msgs)
>   {
> +    merge_tracked_flows(flow_table);
>       struct desired_flow *f, *f_next;
>       LIST_FOR_EACH_SAFE (f, f_next, track_list_node,
>                           &flow_table->tracked_flows) {
>