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

[Mark Michelson]

On 9/8/20 2:07 PM, Han Zhou wrote:
> 
> 
> On Tue, Sep 8, 2020 at 8:55 AM Mark Michelson <mmichels at redhat.com 
> <mailto:mmichels at redhat.com>> wrote:
>  >
>  > 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 <mailto: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.
>  >
> Hi Mark, "delete-after-add/modify" is handled on-the-fly when the flows 
> add/modify & delete are tracked, in track_flow_del(). Here the function 
> merge_tracked_flows() is to handle the "add-after-delete" only. The 
> "add-after-delete" is handled separately here because a deleted flow is 
> removed from the desired flow table already and the following add 
> operation is operating on a new desired_flow instance instead of the 
> deleted one. Maybe I should add this in comments.
> 

Got it. So the list's order is important here. So if the delete comes 
before the add/modify, that indicates the overall intention is to really 
just modify, whereas add/modify before delete means the overall 
intention is to just delete.

>  > > +    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) {
>  > >
>  >