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

Han Zhou hzhou at ovn.org
Tue Sep 8 18:33:17 UTC 2020


On Tue, Sep 8, 2020 at 11:30 AM Mark Michelson <mmichels at redhat.com> wrote:
>
> 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.
>
Exactly :)

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


More information about the dev mailing list